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.

Sign up for GitHub

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

Jump to bottom

feat(cluster.tf): add support to set initial release channel version #1625

Merged
merged 19 commits into from
Jun 2, 2023
Merged

feat(cluster.tf): add support to set initial release channel version #1625

merged 19 commits into from
Jun 2, 2023

Conversation

slimatic
Copy link
Contributor

@slimatic slimatic commented May 1, 2023 •
edited by apeabody

This change adds support for specifying an initial version to use when a release channel is being used. The new logic sets the min_master_version variable to the value of the locally defined master_version when release_channel is null or "UNSPECIFIED". When a release channel is specified, min_master_version is set to the value of kubernetes_version if it is defined, and falls back to the value of master_version otherwise.

Fixes #1460

@slimatic
feat(cluster.tf): add support for setting initial version when using …
8a63c2e 
…release channel

This change adds support for specifying an initial version to use when a release channel is being used.
The new logic sets the `min_master_version` variable to the value of the
locally defined `master_version` when `release_channel` is `null` or `"UNSPECIFIED"`.
When a release channel is specified, `min_master_version` is set to the value of `initial_version`
if it is defined, and falls back to the value of `master_version` otherwise.
@slimatic slimatic requested review from a team, Jberlinsky and ericyz as code owners May 1, 2023 18:54
@google-cla
Copy link

google-cla bot commented May 1, 2023

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.

@apeabody
Copy link
Contributor

apeabody commented May 1, 2023

/gcbrun

@slimatic slimatic mentioned this pull request May 1, 2023
Closed
@apeabody
Copy link
Contributor

apeabody commented May 9, 2023

/gcbrun

apeabody
apeabody requested changes May 9, 2023
View reviewed changes
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 @slimatic!

If we don't have one already, please include one of the test fixtures setting kubernetes_version = "latest" so this new functionally is covered by the Integration tests.

https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/tree/master/test/fixtures

cluster.tf Outdated Show resolved Hide resolved
@slimatic slimatic requested a review from apeabody May 12, 2023 18:20
@apeabody
Copy link
Contributor

/gcbrun

@apeabody
Copy link
Contributor

/gcbrun

@slimatic
Copy link
Contributor Author

/gcbrun

@apeabody fixed the tests and addressed the lint failures.

@apeabody
Copy link
Contributor

/gcbrun

@slimatic
Copy link
Contributor Author

/gcbrun

@apeabody applied make build changes to README.md

@apeabody
Copy link
Contributor

/gcbrun

@apeabody
Copy link
Contributor

/gcbrun

@apeabody
Copy link
Contributor

/gcbrun

@slimatic
Copy link
Contributor Author

@apeabody I noticed the cicd build is failing. Is my attention required on the failure? Please advise.

@apeabody
Copy link
Contributor

/gcbrun

@apeabody
Copy link
Contributor

/gcbrun

@slimatic
Copy link
Contributor Author

Thanks for the contribution @slimatic!

If we don't have one already, please include one of the test fixtures setting kubernetes_version = "latest" so this new functionally is covered by the Integration tests.

https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/tree/master/test/fixtures

@apeabody the requested change has been added.

@slimatic slimatic closed this May 19, 2023
@slimatic slimatic reopened this May 19, 2023
@slimatic
Copy link
Contributor Author

Thanks for the contribution @slimatic!
If we don't have one already, please include one of the test fixtures setting kubernetes_version = "latest" so this new functionally is covered by the Integration tests.
https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/tree/master/test/fixtures

@apeabody the requested change has been added.

@apeabody apologies, I accidentally closed the PR when I was commenting.

@apeabody
Copy link
Contributor

/gcbrun

@slimatic
Copy link
Contributor Author

@apeabody just triggered a new merge of the master branch.
could you also advise on what is needed to successfully merge the pull request?

@apeabody
Copy link
Contributor

@apeabody just triggered a new merge of the master branch. could you also advise on what is needed to successfully merge the pull request?

Thanks @slimatic, I'll re-run the CI. Per merge timeline, what are the potential impacts to existing clusters? Just need to determine if this is OK for immediate minor release now, or if it will need to be a major release with migration documentation.

@apeabody
Copy link
Contributor

/gcbrun

@slimatic
Copy link
Contributor Author

@apeabody just triggered a new merge of the master branch. could you also advise on what is needed to successfully merge the pull request?

Thanks @slimatic, I'll re-run the CI. Per merge timeline, what are the potential impacts to existing clusters? Just need to determine if this is OK for immediate minor release now, or if it will need to be a major release with migration documentation.

@apeabody Thanks for providing some insight on the merge timelines.

@slimatic
Copy link
Contributor Author

@apeabody just triggered a new merge of the master branch. could you also advise on what is needed to successfully merge the pull request?

Thanks @slimatic, I'll re-run the CI. Per merge timeline, what are the potential impacts to existing clusters? Just need to determine if this is OK for immediate minor release now, or if it will need to be a major release with migration documentation.

@apeabody were the following questions were directed to me?

  • "Per merge timeline, what are the potential impacts to existing clusters? "
  • "OK for immediate minor release now, or if it will need to be a major release "

@apeabody
Copy link
Contributor

Hi @slimatic - Thanks, yes, do you have any insight on the potential impacts to existing clusters?

@apeabody
Copy link
Contributor

/gcbrun

@slimatic
Copy link
Contributor Author

Hi @slimatic - Thanks, yes, do you have any insight on the potential impacts to existing clusters?

Yes, based on my tests, making this change will have minimal impact on existing clusters. If the Kubernetes version is modified and applied to an existing cluster, Terraform will upgrade the cluster only if the specified version is greater than the current version. If the version is smaller than the existing version, the terraform apply command will complete successfully without upgrading the cluster version.

bharathkkb
bharathkkb approved these changes Jun 2, 2023
View reviewed changes
Copy link
Member

@bharathkkb bharathkkb left a comment

Choose a reason for hiding this comment

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

Thanks @slimatic and @apeabody. I did a quick test to validate and observed similar behavior as noted by @slimatic so it looks okay to release as minor.

@apeabody
Copy link
Contributor

apeabody commented Jun 2, 2023

Thanks @slimatic and @apeabody. I did a quick test to validate and observed similar behavior as noted by @slimatic so it looks okay to release as minor.

Thanks @bharathkkb - That was above and beyond!

@apeabody
Copy link
Contributor

apeabody commented Jun 2, 2023

/gcbrun

@apeabody
Copy link
Contributor

apeabody commented Jun 2, 2023

Will merge once the CI is all green.

@apeabody apeabody merged commit e522073 into terraform-google-modules:master Jun 2, 2023
4 checks passed
@release-please release-please bot mentioned this pull request Jun 2, 2023
Merged
@michaellzc michaellzc mentioned this pull request Nov 23, 2023
Merged
@yorik yorik mentioned this pull request May 14, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bharathkkb bharathkkb bharathkkb approved these changes

@apeabody apeabody apeabody approved these changes

@Jberlinsky Jberlinsky Awaiting requested review from Jberlinsky

@ericyz ericyz Awaiting requested review from ericyz ericyz is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Module doesnt honor specified GKE master version
3 participants
@slimatic @apeabody @bharathkkb