-
Notifications
You must be signed in to change notification settings - Fork 9
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
feat: update security group logic #422
base: main
Are you sure you want to change the base?
Conversation
@mounika-nalla The upgrade test failed because it has identified an update to the
|
Yes, these are expected because of the version bump of terraform-ibm-landing-zone-vpc from 4.2 to 5.2. The updates involved here are the unique names to the acls, changes to source and destination. I have updated the PR ignoring module.slz_vpc.ibm_is_network_acl.network_acl["vpc-acl”]. I will remove it once the PR is merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mounika-nalla - couple of comments
cb79da0
to
0473454
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double checking this PR - I do not see any moved block to ensure backward compatibility. At the same time, we definitely now that address references would change in the state file from the code in this PR.
The tests are not failing because security group is not created in the default example - hense the security group logic is not exercises.
Could you update the default example to get the module to create a new security group. I suspect that's where we'll see that the change is not backward compatible.
See my comment - found a potential issue when double checking the PR code.
@vburckhardt From my tests, I believe usage of moved blocks is not possible here. the blocks changed are:
For the above block, we are required to use a static value instead of load_balancer.security_group.name and since we are not using any default values, the error - A single static variable reference is required: only attribute access and indexing with constant keys is returned and for the below block, since we are changing from a resource to a module, I’m getting the below noted errors
Please advise if my understanding is incorrect. Also, adding the below module call in default example to call the security_group module to create a new security group ran without any compatibility issues. The upgrade test did not complain about any errors.
|
@mounika-nalla - Thanks. I'd suggest going with a script that is made available in the repository to migrate the state. We can then get the release notes to point to the script. The script would
See also terraform-ibm-modules/terraform-ibm-landing-zone#253 (comment) for something conceptually equivalent. We could have another script as well that use the corresponding schematics mv commands for users using IBM Cloud Schematics. I would name the script migrate_state_currentversion_targetversion.sh and put them in a "migrate" directly in the repo. |
c906351
to
e50ab00
Compare
@mounika-nalla what is the latest with this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this need moved blocks to prevent a breaking change?
We have created a script to manage the state changes as per Vincent's comments above. Using moved blocks is not possible here as we are moving away from using a resource to an attribute. |
@mounika-nalla Ah ok yes - where is the script? |
Its here - migrate/tf_state_migration.sh |
@mounika-nalla What about schematics users? We will need a script to migrate schematics state too, similar to https://github.com/terraform-ibm-modules/terraform-ibm-landing-zone/blob/extend-output/migration/schematics_migration_v4.4.0_to_v5.0.0.sh @vburckhardt We need to figure out how to handle this in the SLZ VSI DA that consumes this module. How will a consumer of a DA via projects know they will need to migrate state. Do we bump the major version and document it in cloud docs? |
…-landing-zone-vsi into modify-sg-logic
…-landing-zone-vsi into modify-sg-logic
/run pipeline |
/run pipeline |
/run pipeline |
Description
Replace security group logic with the security group module
fixes #410
Types of changes in this PR
No release required
Release required
x.x.X
): Change that fixes an issue and is compatible with earlier versions)x.X.x
): Change that adds functionality and is compatible with earlier versions)X.x.x
): Change that is likely incompatible with previous versions)Release notes content
Replace this text with information that users need to know about the bug fixes, features, and breaking changes. This information helps the merger write the commit message that is published in the release notes for the module.
Checklist for reviewers
Merge actions for mergers
Merge by using "Squash and merge".
Use a relevant conventional commit message that is based on the PR contents and any release notes provided by the PR author.
The commit message determines whether a new version of the module is needed, and if so, which semver increment to use (major, minor, or patch).