Skip to content

fix: allow orchestrator_version if auto-upgrade is 'patch' to allow default_node_pool upgrade#302

Merged
lonegunmanb merged 1 commit intoAzure:mainfrom
swisspost:bugfix/issue_301
Feb 18, 2023
Merged

fix: allow orchestrator_version if auto-upgrade is 'patch' to allow default_node_pool upgrade#302
lonegunmanb merged 1 commit intoAzure:mainfrom
swisspost:bugfix/issue_301

Conversation

@aescrob
Copy link
Copy Markdown
Contributor

@aescrob aescrob commented Feb 15, 2023

Describe your changes

-  # - patch, but then the kubernetes_version must not specify a patch number and orchestrator_version must be null
+  # - patch, but then neither the kubernetes_version nor orchestrator_version must specify a patch number

Issue number

#301

Checklist before requesting a review

  • The pr title can be used to describe what this pr did in CHANGELOG.md file
  • I have executed pre-commit on my machine
  • I have passed pr-check on my machine

Thanks for your cooperation!

Copy link
Copy Markdown
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.

LGTM

Copy link
Copy Markdown
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.

Almost LGTM if the e2e test could pass, but would you please update var.automatic_channel_upgrade's description about this new requirement for orchestrator_version when var.automatic_channel_upgrade = patch? Thanks @aescrob

Copy link
Copy Markdown
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 @aescrob for updating this pr, we have some e2e tests failed, one example is:

TestExamplesStartup 2023-02-16T02:25:55Z retry.go:99: Returning due to fatal error: FatalError{Underlying: error while running command: exit status 1; ╷
│ Error: Resource precondition failed
│ 
│   on ../../main.tf line 296, in resource "azurerm_kubernetes_cluster" "main":
│  296:       condition     = local.automatic_channel_upgrade_check
│     ├────────────────
│     │ local.automatic_channel_upgrade_check is false
│ 
│ Either disable automatic upgrades, or only specify up to the minor version
│ when using `automatic_channel_upgrade=patch` or don't specify
│ `kubernetes_version` at all when using
│ `automatic_channel_upgrade=stable|rapid|node-image`. With automatic
│ upgrades `orchestrator_version` must be set to `null`.
╵}
=== CONT  TestExamplesStartup
    apply.go:34: 
        	Error Trace:	/src/test/e2e/apply.go:34
        	            				/src/test/e2e/e2etest.go:57
        	            				/src/test/e2e/e2etest.go:38
        	            				/src/test/e2e/terraform_aks_test.go:22
        	Error:      	Received unexpected error:
        	            	FatalError{Underlying: error while running command: exit status 1; ╷
        	            	│ Error: Resource precondition failed
        	            	│ 
        	            	│   on ../../main.tf line 296, in resource "azurerm_kubernetes_cluster" "main":
        	            	│  296:       condition     = local.automatic_channel_upgrade_check
        	            	│     ├────────────────
        	            	│     │ local.automatic_channel_upgrade_check is false
        	            	│ 
        	            	│ Either disable automatic upgrades, or only specify up to the minor version
        	            	│ when using `automatic_channel_upgrade=patch` or don't specify
        	            	│ `kubernetes_version` at all when using
        	            	│ `automatic_channel_upgrade=stable|rapid|node-image`. With automatic
        	            	│ upgrades `orchestrator_version` must be set to `null`.
        	            	╵}
        	Test:       	TestExamplesStartup

Would you please fix our examples and tests, then verify them by run e2e tests or just run terraform plan for all examples before you commit? Thanks for your understanding!

Btw, in case this pr failed the version update test, which means when the previous version of the example would met errors when it upgrades to this pr version, then we must consider this pr as a breaking change. In that case, I would rather postpone the merge, and release our current version as a new minor version so our users don't need to take the risks to get current unreleased features.

@aescrob
Copy link
Copy Markdown
Contributor Author

aescrob commented Feb 16, 2023

Hi @lonegunmanb - thanks for your support.
I've changed the precondition to still allow orchestrator_version==null. Like this it is not a breaking change and the examples are not required to be fixed.

@aescrob aescrob requested a review from lonegunmanb February 16, 2023 08:11
@aescrob aescrob temporarily deployed to acctests February 17, 2023 01:26 — with GitHub Actions Inactive
Copy link
Copy Markdown
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.

LGTM but I'd like to ask @the-technat for a review.

Copy link
Copy Markdown
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.

Makes sense to me and looks good. Maybe it's worth documenting that the orchestrator_version can be set to null because it should be a non-breaking change and not because it's a recommended approach to configure the module.

Copy link
Copy Markdown
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 @aescrob, LGTM! 🚀

@lonegunmanb lonegunmanb merged commit 890f51b into Azure:main Feb 18, 2023
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