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

auto-upgrade: variable orchestrator_version to null #283

Merged
merged 1 commit into from
Jan 3, 2023

Conversation

zioproto
Copy link
Collaborator

Describe your changes

This is an addition to PR #281

In AKS is possible to upgrade the control plane and the kubernetes nodes independently. The Terraform resource azurerm_kubernetes_cluster exposes this with 2 variables kubernetes_version and orchestrator_version.

In PR #281 the lifecycle validation check takes into account only kubernetes_version.

Issue number

Related to #280

Copy link
Contributor

@the-technat the-technat 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 coming up with this! I totally forgot about the orchestrator_version, but now that you told me about the actually behavior of the nodes, it makes sense to omit this when automatic upgrades are turned on.

Copy link
Member

@lonegunmanb lonegunmanb left a comment

Choose a reason for hiding this comment

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

Thanks @zioproto for opening this pr and thanks @the-technat for participating our discussion. Almost LGTM but I'd like to simplify the logic expression. And since this expression is very complex, would you please add some unit test cases to cover the cases related to var.orchestrator_version? I would recommend adding the new unit tests first to prove your expression works, then try my suggestion with protection from these tests. With the protection from your test cases, we could change or refactor this expression in the future. Thanks @zioproto !

locals.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
@zioproto
Copy link
Collaborator Author

@lonegunmanb please take a look again

@lonegunmanb
Copy link
Member

Hello @zioproto , I'm sorry we've met an issue on our pipeline so we have to upgrade the Github action yaml files in this repo, would you please merge your branch with the latest master branch so we can run the e2e test? Apology for the inconvenience, we're still refactoring the pipeline.

@zioproto zioproto force-pushed the autoupgrade-orchestratorversion branch from 6cf2e9d to 341d6ed Compare January 3, 2023 08:55
@zioproto
Copy link
Collaborator Author

zioproto commented Jan 3, 2023

@lonegunmanb I rebased my commit on top of the current master branch. Thanks

@zioproto zioproto temporarily deployed to acctests January 3, 2023 12:44 — with GitHub Actions Inactive
Copy link
Member

@lonegunmanb lonegunmanb left a comment

Choose a reason for hiding this comment

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

Thanks @zioproto , LGTM! 🚀

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.

3 participants