-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 #1510
feat!: support for gateway api #1510
Conversation
@bharathkkb as mentioned in the issue my change to the terraform provider is now merged and released therefore I was able to add the gateway api to this module as well. @Jberlinsky I'm not sure completely sure about the kitchen tests scope. I just copied the "simple_regional" kitchen test files and adapted them to include the gateway api channel attribute. What I'm not sure about is if it is really necessary to keep all the attribute checks and variables from the "simple_regional" test fixture and example. |
Hi @tuunit , thanks for the contribution. Minor comments:
|
91cf156
to
ad52e25
Compare
Never mind, the bot updated it's state and I had to push another fix anyway. The kitchen / cloudbuild integration and testing and how to add new test suites is not really clear. Maybe we could extend the documentation regarding adding test coverage for new features like in this PR. @bharathkkb @Jberlinsky I don't think this is correct as I already ran |
Unfortunately, I cannot access the cloud build details to check the test failure myself. |
Hi @tuunit , the failure was not relevant to your chance. I'll try to fix it and re-trigger the build after. |
381cc1a
to
5f5f36c
Compare
Minor comments, but LGTM after resolving the conflicts |
*/ | ||
|
||
locals { | ||
cluster_type = "simple-regional" |
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.
Please update the cluster type accordingly
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.
simple-regional -> simple-regional-with-gatewayapi
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.
Done. Can you shortly explain to me why this is necessary? Or is it just to keep everything aligned with some kind of naming convention?
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.
Your understanding is right. In this way, it's easier to identify the testing clusters by its name.
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.
But I guess this now leads to an issue in the integration check as the cluster_name attribute is wrongly set?
https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/pull/1510/files#diff-0e66d26ba66e1418a2d27bff7f636be5977955a71c6222b5e2f9f6f586d21180R15-R25
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.
It was the length of the cluster name exceeding the limit and it should be good now.
@ericyz Thanks a million for fixing the issue. Now I understand what the problem was! It is really unfortunate that normal contributors cannot view the issues of the integration pipeline. |
@tuunit |
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.
LGTM. FYI - @bharathkkb
@Jberlinsky ready for final review 😄 Happy New Year 🥳 |
Co-authored-by: Eric Zhao <[email protected]>
Motivation and Context
As the Gateway API is now GA and part of the Terraform Google Provider (GoogleCloudPlatform/magic-modules#6875) release with version 4.47.0 (https://github.com/hashicorp/terraform-provider-google/releases/tag/v4.47.0).
Therefore I added full support for setting the gateway api channel to this module as well.
Resolves: #1462