Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

resource/aws_transfer_server: Add support for VPC security groups #17539

Merged

Conversation

amadureira
Copy link
Contributor

@amadureira amadureira commented Feb 10, 2021

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #15788.
Closes #19850.
Closes #16556.
Closes #15823.

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSTransferServer_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSTransferServer_basic -timeout 120m
=== RUN   TestAccAWSTransferServer_basic
=== PAUSE TestAccAWSTransferServer_basic
=== CONT  TestAccAWSTransferServer_basic
--- PASS: TestAccAWSTransferServer_basic (258.41s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	258.484s
$ make testacc TESTARGS='-run=testSweepTransferServers'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=testSweepTransferServers -timeout 120m
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	0.056s [no tests to run]
...

@amadureira amadureira requested a review from a team as a code owner February 10, 2021 02:40
@ghost ghost added size/M Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. provider Pertains to the provider itself, rather than any interaction with AWS. service/transfer Issues and PRs that pertain to the transfer service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Feb 10, 2021
@github-actions github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Feb 10, 2021
@github-actions
Copy link

Thank you for your contribution! 🚀

Please note that the CHANGELOG.md file contents are handled by the maintainers during merge. This is to prevent pull request merge conflicts, especially for contributions which may not be merged immediately. Please see the Contributing Guide for additional pull request review items.

Remove any changes to the CHANGELOG.md file and commit them in this pull request to prevent delays with reviewing and potentially merging this pull request.

@amadureira
Copy link
Contributor Author

Hi Folks,
I found some problem in this PR. How can I put WIP label ?

@thiagonache
Copy link

Hi Folks,
I found some problem in this PR. How can I put WIP label ?

you can set in draft @amadureira

@thiagonache
Copy link

Hi Folks,
I found some problem in this PR. How can I put WIP label ?

you can set in draft @amadureira

Also, your PR should not change anything on the files CHANGELOG.md and hidden files.

@amadureira amadureira changed the title Tb aws transfer server security group ids [WIP] Tb aws transfer server security group ids Feb 15, 2021
@amadureira amadureira changed the title [WIP] Tb aws transfer server security group ids Tb aws transfer server security group ids Feb 17, 2021
@ghost ghost added size/S Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Feb 17, 2021
@amadureira amadureira changed the title Tb aws transfer server security group ids [WIP] Tb aws transfer server security group ids Feb 17, 2021
@amadureira
Copy link
Contributor Author

@teamterraform,
I found this problem, but I think it's not a code problem:
https://github.com/hashicorp/terraform-provider-aws/pull/17539/checks?check_run_id=1915113884
Could you ched

.changelog/17496.txt Outdated Show resolved Hide resolved
.changelog/17539.txt Outdated Show resolved Hide resolved
@ewbankkit
Copy link
Contributor

Please add Closes #15788 to the initial comment.

@ghost ghost added size/XS Managed by automation to categorize the size of a PR. and removed size/S Managed by automation to categorize the size of a PR. labels Feb 26, 2021
…points API.

Acceptance test output (failures expected):

% make testacc TEST=./aws TESTARGS='-run=TestAccAWSTransfer_serial/Server/VPC$'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSTransfer_serial/Server/VPC -timeout 180m
=== RUN   TestAccAWSTransfer_serial
=== RUN   TestAccAWSTransfer_serial/Server
=== RUN   TestAccAWSTransfer_serial/Server/VPCAddressAllocationIDs
    resource_aws_transfer_server_test.go:288: Step 3/3 error: Error running apply: exit status 1
        2021/06/29 10:59:43 [DEBUG] Using modified User-Agent: Terraform/0.12.31 HashiCorp-terraform-exec/0.13.3

        Error: error updating Transfer Server (s-45159289697f40d29): InvalidRequestException: Changing Security Group is not supported

          on terraform_plugin_test.tf line 66, in resource "aws_transfer_server" "test":
          66: resource "aws_transfer_server" "test" {

=== RUN   TestAccAWSTransfer_serial/Server/VPC
    resource_aws_transfer_server_test.go:237: Step 3/3 error: Error running apply: exit status 1
        2021/06/29 11:00:57 [DEBUG] Using modified User-Agent: Terraform/0.12.31 HashiCorp-terraform-exec/0.13.3

        Error: error updating Transfer Server (s-e9f52c9d825845e5a): InvalidRequestException: Changing Security Group is not supported

          on terraform_plugin_test.tf line 56, in resource "aws_transfer_server" "test":
          56: resource "aws_transfer_server" "test" {

=== RUN   TestAccAWSTransfer_serial/Server/VPCEndpointID
=== RUN   TestAccAWSTransfer_serial/Server/VPCSecurityGroupIDs
    resource_aws_transfer_server_test.go:340: Step 3/3 error: Error running apply: exit status 1
        2021/06/29 11:03:44 [DEBUG] Using modified User-Agent: Terraform/0.12.31 HashiCorp-terraform-exec/0.13.3

        Error: error updating Transfer Server (s-1e131250f9944c45a): InvalidRequestException: Changing Security Group is not supported

          on terraform_plugin_test.tf line 65, in resource "aws_transfer_server" "test":
          65: resource "aws_transfer_server" "test" {

--- FAIL: TestAccAWSTransfer_serial (411.21s)
    --- FAIL: TestAccAWSTransfer_serial/Server (411.21s)
        --- FAIL: TestAccAWSTransfer_serial/Server/VPCAddressAllocationIDs (185.57s)
        --- FAIL: TestAccAWSTransfer_serial/Server/VPC (58.73s)
        --- PASS: TestAccAWSTransfer_serial/Server/VPCEndpointID (106.22s)
        --- FAIL: TestAccAWSTransfer_serial/Server/VPCSecurityGroupIDs (60.69s)
FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	414.320s
FAIL
make: *** [testacc] Error 1
@github-actions github-actions bot added size/XL Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Jun 29, 2021
…esources should not implement multiple AWS service functionality'.
        Error: error deleting EC2 VPC Endpoint (vpce-09a9ae6b78f2b0571): 1 error occurred:
        	* vpce-09a9ae6b78f2b0571: InvalidVpcEndpoint.NotFound: The Vpc Endpoint Id 'vpce-09a9ae6b78f2b0571' does not exist
@github-actions github-actions bot added the service/ec2 Issues and PRs that pertain to the ec2 service. label Jul 1, 2021
@github-actions github-actions bot removed the provider Pertains to the provider itself, rather than any interaction with AWS. label Jul 2, 2021
@ewbankkit ewbankkit force-pushed the tb-aws_transfer_server_security_group_ids branch from ed45dec to f3cf2ce Compare July 2, 2021 16:37
@ewbankkit ewbankkit changed the title [WIP] Tb aws transfer server security Groupon ids. resource/aws_transfer_server: Add support for VPC security groups Jul 2, 2021
Copy link
Contributor

@ewbankkit ewbankkit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀.

Commercial

% make testacc TEST=./aws TESTARGS='-run=TestAccAWSTransfer_serial/Server'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSTransfer_serial/Server -timeout 180m
=== RUN   TestAccAWSTransfer_serial
=== RUN   TestAccAWSTransfer_serial/Server
=== RUN   TestAccAWSTransfer_serial/Server/disappears
=== RUN   TestAccAWSTransfer_serial/Server/Protocols
=== RUN   TestAccAWSTransfer_serial/Server/SecurityPolicy
=== RUN   TestAccAWSTransfer_serial/Server/UpdateEndpointTypePublicToVPC        
=== RUN   TestAccAWSTransfer_serial/Server/UpdateEndpointTypeVPCEndpointToVPCAddressAllocationIDs
=== RUN   TestAccAWSTransfer_serial/Server/VPCSecurityGroupIDs
=== RUN   TestAccAWSTransfer_serial/Server/basic
=== RUN   TestAccAWSTransfer_serial/Server/UpdateEndpointTypePublicToVPCAddressAllocationIDs
=== RUN   TestAccAWSTransfer_serial/Server/VPCAddressAllocationIDsSecurityGroupIDs
=== RUN   TestAccAWSTransfer_serial/Server/HostKey
=== RUN   TestAccAWSTransfer_serial/Server/Domain
=== RUN   TestAccAWSTransfer_serial/Server/UpdateEndpointTypeVPCEndpointToVPCSecurityGroupIDs
=== RUN   TestAccAWSTransfer_serial/Server/UpdateEndpointTypeVPCToPublic
=== RUN   TestAccAWSTransfer_serial/Server/APIGateway
=== RUN   TestAccAWSTransfer_serial/Server/ForceDestroy
=== RUN   TestAccAWSTransfer_serial/Server/UpdateEndpointTypeVPCEndpointToVPC
=== RUN   TestAccAWSTransfer_serial/Server/VPC
=== RUN   TestAccAWSTransfer_serial/Server/VPCAddressAllocationIDs
=== RUN   TestAccAWSTransfer_serial/Server/VPCEndpointID
=== RUN   TestAccAWSTransfer_serial/Server/APIGatewayForceDestroy
--- PASS: TestAccAWSTransfer_serial (3799.61s)
    --- PASS: TestAccAWSTransfer_serial/Server (3799.61s)
        --- PASS: TestAccAWSTransfer_serial/Server/disappears (151.55s)
        --- PASS: TestAccAWSTransfer_serial/Server/Protocols (283.05s)
        --- PASS: TestAccAWSTransfer_serial/Server/SecurityPolicy (207.53s)
        --- PASS: TestAccAWSTransfer_serial/Server/UpdateEndpointTypePublicToVPC (302.33s)
        --- PASS: TestAccAWSTransfer_serial/Server/UpdateEndpointTypeVPCEndpointToVPCAddressAllocationIDs (241.86s)
        --- PASS: TestAccAWSTransfer_serial/Server/VPCSecurityGroupIDs (75.81s)
        --- PASS: TestAccAWSTransfer_serial/Server/basic (211.54s)
        --- PASS: TestAccAWSTransfer_serial/Server/UpdateEndpointTypePublicToVPCAddressAllocationIDs (369.64s)
        --- PASS: TestAccAWSTransfer_serial/Server/VPCAddressAllocationIDsSecurityGroupIDs (206.68s)
        --- PASS: TestAccAWSTransfer_serial/Server/HostKey (184.54s)
        --- PASS: TestAccAWSTransfer_serial/Server/Domain (187.40s)
        --- PASS: TestAccAWSTransfer_serial/Server/UpdateEndpointTypeVPCEndpointToVPCSecurityGroupIDs (192.16s)
        --- PASS: TestAccAWSTransfer_serial/Server/UpdateEndpointTypeVPCToPublic (233.88s)
        --- PASS: TestAccAWSTransfer_serial/Server/APIGateway (196.55s)
        --- PASS: TestAccAWSTransfer_serial/Server/ForceDestroy (190.65s)
        --- PASS: TestAccAWSTransfer_serial/Server/UpdateEndpointTypeVPCEndpointToVPC (121.09s)
        --- PASS: TestAccAWSTransfer_serial/Server/VPC (184.91s)
        --- PASS: TestAccAWSTransfer_serial/Server/VPCAddressAllocationIDs (284.45s)
        --- PASS: TestAccAWSTransfer_serial/Server/VPCEndpointID (54.44s)
        --- PASS: TestAccAWSTransfer_serial/Server/APIGatewayForceDestroy (228.42s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	3803.238s

@ewbankkit
Copy link
Contributor

@amadureira Thanks for the contribution 🎉 👏.
I went ahead and pushed changes so that SecurityGroupIds can be modified after the Transfer Server has been created.

@ewbankkit ewbankkit removed the service/ec2 Issues and PRs that pertain to the ec2 service. label Jul 2, 2021
@ewbankkit ewbankkit merged commit e367189 into hashicorp:main Jul 2, 2021
@github-actions
Copy link

github-actions bot commented Aug 2, 2021

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. service/transfer Issues and PRs that pertain to the transfer service. size/XL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet