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

fix: remove invalid multi-tenant VPE references #393

Merged
merged 4 commits into from
Sep 22, 2023
Merged

Conversation

vburckhardt
Copy link
Member

@vburckhardt vburckhardt commented Sep 21, 2023

Description

fixes #391
fixes #392

Release required?

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

fix: remove invalid multi-tenant VPE references
fix: ensure idempotent of existing VPEs

BREAKING CHANGE: all existing VPE created in previous version of this module will be deleted and immediately re-created. The IP of the VPE may change as part of this process.

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.

Merge actions for mergers

  • When merging, 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).
  • Merge by using "Squash and merge".

@vburckhardt
Copy link
Member Author

/run pipeline

@vburckhardt
Copy link
Member Author

note: does not cover #394 which will need to be done separately

@vburckhardt
Copy link
Member Author

/run pipeline

1 similar comment
@vburckhardt
Copy link
Member Author

/run pipeline

@vburckhardt
Copy link
Member Author

/run pipeline

@vburckhardt
Copy link
Member Author

/run pipeline

Copy link
Member

@vbontempi vbontempi left a comment

Choose a reason for hiding this comment

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

PR looks ok.
Just a little question to be sure: @vburckhardt the every-mt-type test is not executed with PR ci tests, is this correct?

Copy link
Contributor

@shemau shemau left a comment

Choose a reason for hiding this comment

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

This looks good.

I do not understand why certain cloud_services are being removed and if this leads to work to add databases and secrets-manager back.

Comment on lines -76 to -81
"databases-for-cassandra",
"databases-for-elasticsearch",
"databases-for-enterprisedb",
"databases-for-mongodb",
"databases-for-postgresql",
"databases-for-redis",
Copy link
Contributor

Choose a reason for hiding this comment

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

"iam-svcs",
"is",
"kms",
"resource-controller",
"secrets-manager",
Copy link
Contributor

Choose a reason for hiding this comment

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

secrets-manager is included the list in #396 at https://cloud.ibm.com/docs/vpc?topic=vpc-vpe-supported-services

@vburckhardt
Copy link
Member Author

vburckhardt commented Sep 22, 2023

@shemau - Yes, those should not have been there as, cloud_services variable should only take multi-tenant endpoint. ICD and sm are single tenant (pointing to a specific instance), and as such should be targeted through cloud_service_by_crn

@vbontempi - I had captured that need separately at #394

@vburckhardt vburckhardt merged commit a17b643 into main Sep 22, 2023
2 checks passed
@vburckhardt vburckhardt deleted the every-mt-vpe branch September 22, 2023 15:20
@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 3.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@shemau
Copy link
Contributor

shemau commented Sep 22, 2023

@shemau - Yes, those should not have been there as, cloud_services variable should only take multi-tenant endpoint. ICD and sm are single tenant (pointing to a specific instance), and as such should be targeted through cloud_service_by_crn

@vbontempi - I had captured that need separately at #394

Thanks. For the record, coming soon, the ICD postgresql example will cover VPE wrt to the single tenant instance that is created.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vpe list is not stable invalid global vpe crns
4 participants