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

refactor: the vsi-map key to use the subnet-name #610

Merged
merged 55 commits into from
May 13, 2024
Merged

refactor: the vsi-map key to use the subnet-name #610

merged 55 commits into from
May 13, 2024

Conversation

Aashiq-J
Copy link
Member

@Aashiq-J Aashiq-J commented Jan 18, 2024 •
edited

Description

Issue: https://github.ibm.com/GoldenEye/issues/issues/7108

Release required?

  • No release
  • Patch release (x.x.X)
  • Minor release (x.X.x)
  • Major release (X.x.x)
Release notes content

This is a breaking change, vsis and volumes will get recreated. To prevent recreation use the script in the update folder to update the state file to the new map structure.

Run the pipeline

If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.

Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:

/run pipeline

Checklist for reviewers

  • If relevant, a test for the change is included or updated with this PR.
  • If relevant, documentation for the change is included or updated with this PR.

For mergers

  • Use a conventional commit message to set the release level. Follow the guidelines.
  • Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
  • Use the Squash and merge option.

@Aashiq-J
Copy link
Member Author

The reason for using the subnet name instead of the subnet-id as a key for the vsi-map is because the subnet-id is only known after the apply operation. Therefore, it cannot be used as a key in the vsi-map.

@Aashiq-J
Copy link
Member Author

/run pipeline

@Aashiq-J
Copy link
Member Author

/run pipeline

@Aashiq-J
Copy link
Member Author

/run pipeline

@Aashiq-J Aashiq-J changed the title feat: refactor the vsi code to update the vsi-map key refactor: the vsi-map key to use the subnet-name Jan 18, 2024
@Aashiq-J
Copy link
Member Author

/run pipeline

@ocofaigh
Copy link
Member

@Aashiq-J I see you skipped the upgrade test already. Did you run it though? Can we see the result of it?

@ocofaigh
Copy link
Member

@Aashiq-J Can you also add this to the deep dive next week so we can discuss how to handle the breaking change

@Aashiq-J
Copy link
Member Author

@Aashiq-J I see you skipped the upgrade test already. Did you run it though? Can we see the result of it?

@ocofaigh , This is the test in which upgrade test failed.
https://github.com/terraform-ibm-modules/terraform-ibm-landing-zone-vsi/actions/runs/7565475375/job/20601352555

@Aashiq-J
Copy link
Member Author

/run pipeline

@Aashiq-J
Copy link
Member Author

/run pipeline

@ocofaigh
Copy link
Member

NOTE: I have created a pre-release from this branch at the current commit level so it can be shared with some teams for feedback -> https://github.com/terraform-ibm-modules/terraform-ibm-landing-zone-vsi/releases/tag/v4.0.0-rc1

ocofaigh
ocofaigh requested changes May 2, 2024
View reviewed changes
Copy link
Member

@ocofaigh ocofaigh left a comment •
edited

Choose a reason for hiding this comment

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

See latest comment. Also I think when passing multiple VPCs the value can’t have spaces between VPC IDs?
When I tried -v "r006-6a834173-7ae1-4c1a-8caf-e2c79c5e1d0f, r006-869a47d2-3ca3-4f05-850e-c6c3ced89c13" it failed with:

Failed to get vpc.
Error code: not_found
Error message: VPC not found
Error target name: id, type: parameter
Error target value:  r006-869a47d2-3ca3-4f05-850e-c6c3ced89c13
More information: id is not valid
Trace ID: be308ed5-c28d-495d-af9e-90ef2c0e7b65

But it worked with "r006-6a834173-7ae1-4c1a-8caf-e2c79c5e1d0f,r006-869a47d2-3ca3-4f05-850e-c6c3ced89c13"
I think it should not be too hard to support the space scenario? Or if not, we should at least fail hard if it has a space saying its wrong format used.

update/update-version.md Outdated Show resolved Hide resolved
update/update_v3.x.x_to_v4.x.x.sh Outdated Show resolved Hide resolved
update/update_v3.x.x_to_v4.x.x.sh Outdated Show resolved Hide resolved
update/update_v3.x.x_to_v4.x.x.sh Outdated Show resolved Hide resolved
update/update_v3.x.x_to_v4.x.x.sh Outdated Show resolved Hide resolved
update/update_v3.x.x_to_v4.x.x.sh Outdated Show resolved Hide resolved
update/update-version.md Outdated Show resolved Hide resolved
update/update-version.md Outdated Show resolved Hide resolved
update/update-version.md Outdated Show resolved Hide resolved
update/update-version.md Outdated Show resolved Hide resolved
ocofaigh
ocofaigh requested changes May 2, 2024
View reviewed changes
Copy link
Member

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

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

see comments for schematics docs

update/schematics_update_v3.x.x_to_v4.x.x.sh Outdated Show resolved Hide resolved
update/schematics_update_v3.x.x_to_v4.x.x.sh Outdated Show resolved Hide resolved
update/update-version.md Outdated Show resolved Hide resolved
update/update-version.md Show resolved Hide resolved
update/update-version.md Outdated Show resolved Hide resolved
update/update-version.md Outdated Show resolved Hide resolved
update/schematics_update_v3.x.x_to_v4.x.x.sh Outdated Show resolved Hide resolved
update/schematics_update_v3.x.x_to_v4.x.x.sh Outdated Show resolved Hide resolved
update/update-version.md Outdated Show resolved Hide resolved
update/update-version.md Outdated Show resolved Hide resolved
@Aashiq-J Aashiq-J requested a review from ocofaigh May 3, 2024 08:19
@ocofaigh ocofaigh requested a review from SirSpidey May 7, 2024 08:17
@ocofaigh
Copy link
Member

ocofaigh commented May 7, 2024

I am happy with the testing to approve the PR from technical stance. Have added @SirSpidey to review the docs

SirSpidey and others added 2 commits May 10, 2024 12:04
@SirSpidey
Edits to new instructions (#667)
400320f 
* Edits to new instructions

* Update afer review

* Update version numbers

Use major version indicator only
@Aashiq-J
rename script
9affd48
@Aashiq-J
Copy link
Member Author

/run pipeline

SirSpidey
SirSpidey previously requested changes May 10, 2024
View reviewed changes
Copy link
Member

@SirSpidey SirSpidey left a comment

Choose a reason for hiding this comment

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

one small change to comments in the script. Otherwise good to go.

Aashiq-J and others added 2 commits May 12, 2024 00:24
@Aashiq-J
Copy link
Member Author

/run pipeline

@Aashiq-J
Copy link
Member Author

/run pipeline

@ocofaigh
Copy link
Member

/run pipeline

@ocofaigh ocofaigh dismissed SirSpidey’s stale review May 13, 2024 10:39

Allens changes are merged into this PR

@ocofaigh ocofaigh merged commit 3790100 into main May 13, 2024
2 checks passed
@ocofaigh ocofaigh deleted the refactor branch May 13, 2024 10:39
@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 4.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

@ocofaigh ocofaigh ocofaigh approved these changes

@SirSpidey SirSpidey SirSpidey left review comments

@argeiger argeiger Awaiting requested review from argeiger

@toddgiguere toddgiguere Awaiting requested review from toddgiguere toddgiguere is a code owner

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Aashiq-J @ocofaigh @terraform-ibm-modules-ops @SirSpidey