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!: support for gateway api for safer cluster variants #1523

Conversation

lauraseidler
Copy link
Contributor

Follow up to #1510

@ericyz
Copy link
Collaborator

ericyz commented Jan 8, 2023

Re-run the checks

@lauraseidler
Copy link
Contributor Author

Re-run the checks

Can't, unfortunately, I don't think I have the necessary permissions

@ericyz
Copy link
Collaborator

ericyz commented Jan 9, 2023

Re-run the checks

Can't, unfortunately, I don't think I have the necessary permissions

I have re-triggered it for you.

@lauraseidler
Copy link
Contributor Author

Still no luck it seems :(

@apeabody
Copy link
Contributor

apeabody commented Jan 9, 2023

Thanks for the contribution @lauraseidler! - I suspect the test failure is related to #1525

@lauraseidler
Copy link
Contributor Author

Could be - it's really hard to figure out without having access to the builds. I guess I'll wait for the fix, then rebase and see if it works then.

@tuunit
Copy link
Contributor

tuunit commented Jan 10, 2023

@lauraseidler sorry that I missed to add this feature to the safer cluster module as well in the initial PR

@lauraseidler
Copy link
Contributor Author

@apeabody Thanks for the fix!
@tuunit no worries, I think most people do

@ericyz ericyz changed the title feat: support for gateway api for safer cluster variants feat!: support for gateway api for safer cluster variants Jan 10, 2023
@ericyz
Copy link
Collaborator

ericyz commented Jan 10, 2023

Thanks @lauraseidler for the contribution. LGTM overall. Just a minor comment here. Could you please also update the modules/safer-cluster/variables.tf to pin the Google provider version >= 4.47.0

@lauraseidler
Copy link
Contributor Author

lauraseidler commented Jan 11, 2023

@ericyz the google provider version is currently not in there at all - do you want me to add it? I think since the safer cluster variants just use the beta private cluster module with some variables set to defaults, it's not necessary. However, I see that the versions.tf files in some modules that do specify them haven't been updated - maybe those need to be updated instead https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/blob/master/autogen/main/versions.tf.tmpl#L27?

@lauraseidler
Copy link
Contributor Author

Yeah I actually think this would break if released as is, as the gateway API support is implemented for the beta clusters, but the provider constraint is not there - I pushed the increase, so probably it's a good idea to merge this before #1512.

@ericyz
Copy link
Collaborator

ericyz commented Jan 15, 2023

Yeah I actually think this would break if released as is, as the gateway API support is implemented for the beta clusters, but the provider constraint is not there - I pushed the increase, so probably it's a good idea to merge this before #1512.

Thanks @lauraseidler . Yes, it implicitly uses other modules. That's all good

@ericyz
Copy link
Collaborator

ericyz commented Jan 15, 2023

LGTM and will approve once the CI passed

@comment-bot-dev
Copy link

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

Copy link
Contributor

@apeabody apeabody left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @lauraseidler!

@apeabody apeabody merged commit 912da8c into terraform-google-modules:master Jan 17, 2023
@lauraseidler lauraseidler deleted the feature/gateway-safer-cluster branch January 19, 2023 08:44
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

5 participants