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.

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

SetSubnets not allowed on LB type network (supersedes #1941) #2293

Closed
wants to merge 2 commits into from
Closed

SetSubnets not allowed on LB type network (supersedes #1941) #2293

wants to merge 2 commits into from

Conversation

catsby
Copy link
Contributor

@catsby catsby commented Nov 14, 2017

Continuation of #1941

  • document this in the lb documentation
  • mention that terraform taint is the way to do change the subnets in our error message
  • update the check on subnet update to only fire if it's not a new resource

@catsby catsby requested a review from radeksimko November 14, 2017 21:26
@catsby catsby mentioned this pull request Nov 14, 2017
Closed
@radeksimko radeksimko added the bug Addresses a defect in current functionality. label Nov 14, 2017
radeksimko
radeksimko approved these changes Nov 14, 2017
View reviewed changes
aws/resource_aws_lb.go
if d.Get("load_balancer_type").(string) == "network" {
// Load Balancers of type 'network' cannot update their subnets at this
// time.
return fmt.Errorf("Unable to update subnets for loadbalancer %s, updating subnets is not supported for type network. Please use the 'taint' command to force a recreation of this resource", d.Id())
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick (read as I'm happy to land this as is): Could we possibly use the new CustomizeDiff to mark subnets as ForceNew in this case? https://github.com/hashicorp/terraform/pull/14887/files#diff-456d1e45f8a8d1585e8228597b9a2176R2845 instead of instructing the user to taint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL about CustomizeDiff! I've opened #2310 as a replacement

@radeksimko radeksimko added the size/S Managed by automation to categorize the size of a PR. label Nov 15, 2017
@catsby
Copy link
Contributor Author

catsby commented Nov 15, 2017

I'm going to reopen this with a fancy new CustomizeDiff

@catsby catsby closed this Nov 15, 2017
@catsby catsby mentioned this pull request Nov 15, 2017
Merged
2 tasks
@catsby
Copy link
Contributor Author

catsby commented Nov 15, 2017

Replacement at #2310

@bflad bflad deleted the pr-1941 branch September 14, 2018 15:21
@ghost
Copy link

ghost commented Apr 3, 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 ghost locked and limited conversation to collaborators Apr 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@radeksimko radeksimko radeksimko approved these changes

Assignees
No one assigned
Labels
bug Addresses a defect in current functionality. size/S Managed by automation to categorize the size of a PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@catsby @radeksimko @dblooman