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.

Sign up for GitHub

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

Jump to bottom

New Resource: aws_dx_connection #2173

Merged

radeksimko merged 3 commits into hashicorp:master from atsushi-ishibashi:resource_dx_connection

Nov 7, 2017

Contributor

atsushi-ishibashi commented Nov 4, 2017

make testacc TEST=./aws TESTARGS='-run=TestAccAwsDxConnection_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAwsDxConnection_basic -timeout 120m
=== RUN   TestAccAwsDxConnection_basic
--- PASS: TestAccAwsDxConnection_basic (64.31s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	64.364s

Contributor Author

atsushi-ishibashi commented Nov 4, 2017

After this PR and #2154 will be merged, I'll develop aws_dx_lag_association to associate connection with lag.

Ninir added the enhancement label

radeksimko suggested changes

View reviewed changes

Member

radeksimko left a comment

Thanks for this PR @atsushi-ishibashi
I left you some comments there. Let me know if anything's unclear.

Also I assume you'll need to rebase the branch since your other DX PR was just merged and conflicts with this one.

aws/resource_aws_dx_connection.go Outdated

    
              				Required: true,

              				ForceNew: true,

              			},

              			"band_width": &schema.Schema{

Member

radeksimko Nov 7, 2017

nitpick, but I never saw this spelled as "band width" and the API also uses 1 word - bandwidth - mind changing that?

aws/resource_aws_dx_connection.go Outdated

    
              	}

              	resp, err := conn.DescribeConnections(input)

              	if err != nil {

              		d.SetId("")

Member

radeksimko Nov 7, 2017

As mentioned in your other DX PR - we shouldn't remove a resource from the state on any error and there's no relevant error that would signal missing resource: http://docs.aws.amazon.com/directconnect/latest/APIReference/API_DescribeConnections.html#API_DescribeConnections_Errors

aws/resource_aws_dx_connection.go Outdated

    
              		return err

              	}

              	if len(resp.Connections) != 1 {

              		d.SetId("")

Member

radeksimko Nov 7, 2017

Same here - we should only ever remove the resource from state if len(resp.Connections) < 1 (and not error out in such case).

aws/resource_aws_dx_connection.go Outdated

    
              		return fmt.Errorf("[ERROR] Number of DX Connection (%s) isn't one, got %d", connectionId, len(resp.Connections))

              	}

              	if d.Id() != *resp.Connections[0].ConnectionId {

              		d.SetId("")

Member

radeksimko Nov 7, 2017

Same here ^

aws/resource_aws_dx_connection.go Outdated

    
              	}

              	deleteStateConf := &resource.StateChangeConf{

              		Pending:    []string{directconnect.ConnectionStatePending, directconnect.ConnectionStateOrdering, directconnect.ConnectionStateAvailable, directconnect.ConnectionStateRequested},

              		Target:     []string{directconnect.ConnectionStateDeleted, directconnect.ConnectionStateDeleting},

Member

radeksimko Nov 7, 2017

Any particular reason to treat directconnect.ConnectionStateDeleting as target here?

aws/resource_aws_dx_connection_test.go Outdated

    
                  resource "aws_dx_connection" "hoge" {

                    name = "tf-dx-%s"

                    band_width = "1Gbps"

                    location = "EqDC2"

Member

radeksimko Nov 7, 2017

As mentioned in your other PR - we run all acceptance tests in us-west-2, so we'll need to set a different location here, e.g. EqSe2

aws/validators.go Outdated

    
            @@ -1651,3 +1651,21 @@ func validateCognitoRoles(v map[string]interface{}, k string) (errors []error) {
          
              	return

              }

              func validateDxConnectionBandWidth(v interface{}, k string) (ws []string, errors []error) {

Member

radeksimko Nov 7, 2017

How do you feel about renaming the other validation function from the previous PR and reusing it here? It seems to be validating the exact same thing.

Contributor Author

atsushi-ishibashi Nov 7, 2017

I think validateDxConnectionBandWidth is better name so I replace dx_lag validate func with it.

aws/resource_aws_dx_connection_test.go Outdated

    
              			return err

              		}

              		for _, v := range resp.Connections {

              			if *v.ConnectionId == rs.Primary.ID && !(*v.ConnectionState == directconnect.ConnectionStateDeleted || *v.ConnectionState == directconnect.ConnectionStateDeleting) {

Member

radeksimko Nov 7, 2017

directconnect.ConnectionStateDeleting shouldn't be treated as "gone" here, IMO

radeksimko added new-resource waiting-response and removed enhancement labels

atsushi-ishibashi force-pushed the resource_dx_connection branch from 2887129 to c7df691 Compare

November 7, 2017 15:51

atsushi-ishibashi added 2 commits

November 8, 2017 01:20


          New Resource: aws_dx_connection

c0fb5fb


          Modify constant

fd2e1a3

atsushi-ishibashi force-pushed the resource_dx_connection branch from c7df691 to fd2e1a3 Compare

November 7, 2017 16:21


          Reflect reviews

radeksimko removed the waiting-response label

radeksimko approved these changes

View reviewed changes

Member

radeksimko left a comment

Thanks 👍 🚀

radeksimko merged commit 0c39144 into hashicorp:master

atsushi-ishibashi deleted the resource_dx_connection branch

December 13, 2017 15:16

ghost commented Apr 10, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

ghost locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels