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

More accurate control of create before destroy behaviors #35

Merged
merged 9 commits into from
Jul 7, 2022
Merged

Conversation

Nuru
Copy link
Contributor

@Nuru Nuru commented Jun 25, 2022

note

README and code are (hopefully) final.

This will be released as v2.0.0-rc1 due to changed defaults, requirement for Terraform 1.0, and possible service interruption when upgrading. Migration document TBD.

what

  • Make create_before_destroy default to true for security groups
  • Introduce preserve_security_group_id to control replacement of security group when rules change

why

references

@Nuru Nuru added wip Work in Progress: Not ready for final review or merge major Breaking changes (or first stable release) labels Jun 25, 2022
@Nuru
Copy link
Contributor Author

Nuru commented Jun 25, 2022

/test all

@Nuru
Copy link
Contributor Author

Nuru commented Jun 25, 2022

/test all

@Nuru
Copy link
Contributor Author

Nuru commented Jun 25, 2022

/test all

main.tf Outdated Show resolved Hide resolved
Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

please see comments

@Nuru Nuru requested a review from aknysh June 27, 2022 04:18
@Nuru
Copy link
Contributor Author

Nuru commented Jun 27, 2022

/test all

@Nuru Nuru marked this pull request as ready for review July 7, 2022 01:24
@Nuru Nuru requested review from a team as code owners July 7, 2022 01:24
@Nuru
Copy link
Contributor Author

Nuru commented Jul 7, 2022

/test all

@Nuru
Copy link
Contributor Author

Nuru commented Jul 7, 2022

/test all

test/src/go.mod Outdated Show resolved Hide resolved
This is particularly important because a security group cannot be destroyed while it is associated with
a resource (e.g. a load balancer), but "destroy before create" behavior causes Terraform
to try to destroy the security group before disassociating it from associated resources,
so plans fail to apply with the error
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
so plans fail to apply with the error
so plans fail with the error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Terraform successfully creates the plan, but the plan fails to apply.

README.yaml Outdated Show resolved Hide resolved
README.yaml Outdated Show resolved Hide resolved
README.yaml Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

LGTM, a few nitpicks

osterman
osterman previously approved these changes Jul 7, 2022
@osterman
Copy link
Member

osterman commented Jul 7, 2022

Great depiction of the problem at a high-level. This is a great addition to the README.

@Nuru Nuru added the no-release Do not create a new release (wait for additional code changes) label Jul 7, 2022
@Nuru Nuru requested a review from aknysh July 7, 2022 19:40
@Nuru
Copy link
Contributor Author

Nuru commented Jul 7, 2022

/test all

@Nuru Nuru merged commit a7ff89b into master Jul 7, 2022
@Nuru Nuru deleted the rule-cbd branch July 7, 2022 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major Breaking changes (or first stable release) no-release Do not create a new release (wait for additional code changes) wip Work in Progress: Not ready for final review or merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws_security_group_rule create_before_destroy triggers bug in provider
3 participants