New Resource: aws_dx_lag #2154
New Resource: aws_dx_lag #2154
Conversation
atsushi-ishibashi
commented
Nov 2, 2017
atsushi-ishibashi
commented
|
Related #876 |
Ninir
added
the
enhancement
radeksimko
added
new-resource
There was a problem hiding this comment.
Hi @atsushi-ishibashi
thanks for the PR.
I left you some comments - some are more important, some less. Let me know if you need any further clarification.
aws/resource_aws_dx_lag.go
Outdated
| } | ||
| if len(resp.Lags) != 1 { | ||
| d.SetId("") | ||
| return fmt.Errorf("[ERROR] Number of DX Lag (%s) isn't one, got %d", lagId, len(resp.Lags)) |
There was a problem hiding this comment.
I'm ok with erroring out if we get != 1 lags, but I don't think it's a reason for removing the LAG from state. We should only be removing it from state if it's not found - i.e. len(resp.Lags) < 1
aws/resource_aws_dx_lag.go
Outdated
| } | ||
| resp, err := conn.DescribeLags(input) | ||
| if err != nil { | ||
| d.SetId("") |
There was a problem hiding this comment.
I can't find any particular error that would mean LAG is gone in this context: http://docs.aws.amazon.com/directconnect/latest/APIReference/API_DescribeLags.html#c
so I don't think error here is a reason for removing the LAG from state. 🤔
aws/resource_aws_dx_lag.go
Outdated
| } | ||
| if d.Id() != *resp.Lags[0].LagId { | ||
| d.SetId("") | ||
| return fmt.Errorf("[ERROR] DX Lag (%s) not found", lagId) |
There was a problem hiding this comment.
It makes sense to error out if wrong ID was found (not sure how/why would that happen, but strange things do happen sometimes), but I'm not sure if it's the reason for removing LAG from state.
aws/resource_aws_dx_lag.go
Outdated
| } | ||
| deleteStateConf := &resource.StateChangeConf{ | ||
| Pending: []string{directconnect.LagStateAvailable, directconnect.LagStateRequested, directconnect.LagStatePending}, | ||
| Target: []string{directconnect.LagStateDeleted, directconnect.LagStateDeleting}, |
There was a problem hiding this comment.
Is there any reason we should treat directconnect.LagStateDeleting as Target state here?
aws/resource_aws_dx_lag.go
Outdated
| Type: schema.TypeString, | ||
| Required: true, | ||
| }, | ||
| "band_width": &schema.Schema{ |
There was a problem hiding this comment.
Nitpick, but is there any reason to drift away from the name in the API? i.e. can't we just call it connections_bandwidth?
There was a problem hiding this comment.
I totally agree with your reviews!
aws/resource_aws_dx_lag.go
Outdated
| Required: true, | ||
| ForceNew: true, | ||
| }, | ||
| "num_connections": &schema.Schema{ |
There was a problem hiding this comment.
Nitpick, but is there any reason to drift away from the name in the API? i.e. can't we just call it number_of_connections ?
aws/resource_aws_dx_lag_test.go
Outdated
| resource "aws_dx_lag" "hoge" { | ||
| name = "tf-dx-lag-%s" | ||
| band_width = "1Gbps" | ||
| location = "EqDC2" |
There was a problem hiding this comment.
Do you mind changing the location to a one which is available in us-west-2 as all of our tests run in that region? e.g. EqSe2
radeksimko
added
the
waiting-response
radeksimko
removed
the
waiting-response
aws/resource_aws_dx_lag.go
Outdated
| } | ||
| if len(resp.Lags) != 1 { | ||
| if len(resp.Lags) < 1 { | ||
| d.SetId("") |
There was a problem hiding this comment.
I think in this case (when Lag was removed) we don't want to error out - at least it's a convention not to do that.
Also - which is rather a nitpick - we can move this condition above or below the other condition and avoid the nesting & indentation so it's easier to read.
There was a problem hiding this comment.
I understood the convention 🙇
|
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
