-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: add enable_k8s_beta_apis support #2387
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
base: main
Are you sure you want to change the base?
feat: add enable_k8s_beta_apis support #2387
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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.
Thanks for the contribution @maikelpoot!
@@ -33,6 +33,13 @@ resource "google_container_cluster" "primary" { | |||
network = "projects/${local.network_project_id}/global/networks/${var.network}" | |||
deletion_protection = var.deletion_protection | |||
|
|||
dynamic "enable_k8s_beta_apis" { |
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 add to one of the existing examples (/examples) so this is covered by our tests.
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.
Damn, Just noticed i read your comment wrong, I added an additional test (by copying simple_regional_private
to simple_regional_private_with_beta_apis
) instead of adding it to an existing example.
Is that ok, or should i add it to the simple_regional_private
example.
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.
Hi @maikelpoot! - It's fine to add as a new example, but more complex as that will need to be onboard as a test. In particular:
- A new ./test/fixtures to apply
- A new ./test/integration to validate
- Add the required apply, validate, teardown for the next example to ./build/int.cloudbuild.yaml
- Etc
Either direction you choose is fine.
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.
hi @apeabody,
As tempting as that sounds, i removed the extra example and applied the changes to the simple_regional_private
example.
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.
Sounds good, you will need to pass a value for enable_k8s_beta_apis
into the example fixture from here: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/blob/main/test/fixtures/simple_regional_private/example.tf
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.
Added a beta api to the default value of the fixture variable. As an empty default value doesn't test anything.
But this does mean that if this api is promoted within later k8s/gke version it could fail on this value.
…for simple_regional_private
…terraform-google-kubernetes-engine into feature/enable_k8s_beta_apis
This PR will expose the enable_k8s_beta_apis:enabled_apis block of the
google_container_cluster
resource as a list of strings variable.Example:
Tested as a local checkout against a cluster that had api's enabled with gcloud cli