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

Feat: Multiple registry projects #815

Merged
merged 9 commits into from
Feb 16, 2021
Merged

Feat: Multiple registry projects #815

merged 9 commits into from
Feb 16, 2021

Conversation

rux616
Copy link
Contributor

@rux616 rux616 commented Feb 7, 2021

In our organization, we have a few projects that we put container images into depending on the team, and sometimes a cluster needs access to more than one of them. Right now we use a custom module that isn't a wrapper for this one, but I'd like to change that. This PR would allow that to happen, but it does mean a breaking change for those who utilize the variable as it is currently defined.

I've also updated the tests to handle the change of the variable name from registry_project_id to registry_project_ids and the type now being list(string). The workload-metadata-config-local integration test (the only test to reference the former registry_project_id variable) passes locally as well. That being said, I've not used InSpec before and am not 100% sure everything's correct regarding my changes, so if it's decided that this PR merits inclusion, I'd appreciate a closer look at that part of the PR just to make sure nothing's broken.

@rux616 rux616 requested review from bharathkkb, Jberlinsky and a team as code owners February 7, 2021 10:02
@comment-bot-dev
Copy link

comment-bot-dev commented Feb 7, 2021

Thanks for the PR! 🚀
✅ Lint checks have passed.

@morgante
Copy link
Contributor

morgante commented Feb 8, 2021

/gcbrun

Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

For backwards compatibility, could you keep the existing registry_project_id as well? (Otherwise we'll have to wait until next major release.)

@rux616
Copy link
Contributor Author

rux616 commented Feb 8, 2021

For backwards compatibility, could you keep the existing registry_project_id as well? (Otherwise we'll have to wait until next major release.)

Sure. I'm thinking that I could rename the registry_project_ids variable to something like additional_registry_project_ids for that.

Alternatively, not renaming the new registry_project_ids variable and simply adding some verbiage to registry_project_id saying that it's still functional but planned to be superseded by registry_project_ids could be a good idea. Thoughts?

@morgante
Copy link
Contributor

morgante commented Feb 8, 2021

Alternatively, not renaming the new registry_project_ids variable and simply adding some verbiage to registry_project_id saying that it's still functional but planned to be superseded by registry_project_ids could be a good idea. Thoughts?

Right, that's what I had in mind.

@rux616
Copy link
Contributor Author

rux616 commented Feb 8, 2021

@morgante The requested changes have been made. I've added the registry_project_id variable back in, and made its description a deprecation warning. I've left the tests as they are for the new registry_project_ids variable.

Once this PR is merged, I'll create another one that will actually remove the registry_project_id variable and can be left to sit until the next major version is imminent.

@morgante morgante merged commit 5562cd6 into terraform-google-modules:master Feb 16, 2021
@rux616 rux616 deleted the feat/multiple-registry-projects branch February 24, 2021 16:47
CPL-markus pushed a commit to WALTER-GROUP/terraform-google-kubernetes-engine that referenced this pull request Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants