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

azurerm_kubernetes_cluster - add support for node_os_channel_upgrade #22187

Merged
merged 5 commits into from Jun 20, 2023

Conversation

stephybun
Copy link
Member

Tests are still running.

Superseded #21386
Related to #20171

Copy link
Member

@mbfrahry mbfrahry 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
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

JK, I take it back. Almost there with a minor addition to fix an existing test

autoChannelUpgrade := d.Get("automatic_channel_upgrade").(string)
nodeOsChannelUpgrade := d.Get("node_os_channel_upgrade").(string)
if autoChannelUpgrade != "" {
if autoChannelUpgrade == string(managedclusters.UpgradeChannelNodeNegativeimage) && nodeOsChannelUpgrade != string(managedclusters.NodeOSUpgradeChannelNodeImage) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this wants to be

Suggested change
if autoChannelUpgrade == string(managedclusters.UpgradeChannelNodeNegativeimage) && nodeOsChannelUpgrade != string(managedclusters.NodeOSUpgradeChannelNodeImage) {
if autoChannelUpgrade == string(managedclusters.UpgradeChannelNodeNegativeimage) && string(managedclusters.NodeOSUpgradeChannelNodeImage) != "" && nodeOsChannelUpgrade != string(managedclusters.NodeOSUpgradeChannelNodeImage) {

to get around

=== CONT  TestAccKubernetesCluster_upgradeChannel
testcase.go:110: Step 5/10 error: Error running apply: exit status 1
Error: `node_os_channel_upgrade` must be set to `NodeImage` if `automatic_channel_upgrade` is set to `node-image`
with azurerm_kubernetes_cluster.test,
on terraform_plugin_test.tf line 22, in resource "azurerm_kubernetes_cluster" "test":
22: resource "azurerm_kubernetes_cluster" "test" {
--- FAIL: TestAccKubernetesCluster_upgradeChannel (795.62s)

Copy link
Member

Choose a reason for hiding this comment

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

Whoops! Checked the api here and passing in

"autoUpgradeProfile": {
  "upgradeChannel": "node-image"
},

returns

"autoUpgradeProfile": {
  "upgradeChannel": "node-image",
  "nodeOSUpgradeChannel": "NodeImage"
},

So my old comment is wrong

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

LGTM once tests pass! My old review comment was wrong 🙈

autoChannelUpgrade := d.Get("automatic_channel_upgrade").(string)
nodeOsChannelUpgrade := d.Get("node_os_channel_upgrade").(string)
if autoChannelUpgrade != "" {
if autoChannelUpgrade == string(managedclusters.UpgradeChannelNodeNegativeimage) && nodeOsChannelUpgrade != string(managedclusters.NodeOSUpgradeChannelNodeImage) {
Copy link
Member

Choose a reason for hiding this comment

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

Whoops! Checked the api here and passing in

"autoUpgradeProfile": {
  "upgradeChannel": "node-image"
},

returns

"autoUpgradeProfile": {
  "upgradeChannel": "node-image",
  "nodeOSUpgradeChannel": "NodeImage"
},

So my old comment is wrong

@stephybun
Copy link
Member Author

Thanks for the review @mbfrahry! Relevant tests are passing so this should be good to merge.

@stephybun stephybun merged commit 5e1828a into main Jun 20, 2023
13 checks passed
@stephybun stephybun deleted the f/aks-node-os-upgrade branch June 20, 2023 09:14
@github-actions github-actions bot added this to the v3.62.0 milestone Jun 20, 2023
stephybun added a commit that referenced this pull request Jun 20, 2023
ziyeqf pushed a commit to ziyeqf/terraform-provider-azurerm that referenced this pull request Jun 26, 2023
…de` (hashicorp#22187)

* add support for node os upgrade channel

* goimports

* remove duplicated setting on autoUpgradeProfile

* fix tests

* reference temp name for rotation properly and try different sku for dedicated host
ziyeqf pushed a commit to ziyeqf/terraform-provider-azurerm that referenced this pull request Jun 26, 2023
@stevehipwell
Copy link

@stephybun @mbfrahry this is a breaking change for anyone who was setting automatic_channel_upgrade to node-image as if they upgrade the provider version to v3.6.0 or greater the once working code will fail. If you really can't default node_os_channel_upgrade to NodeImage in the case where it's not explicitly set (I have no idea why not) then this code change should have triggered a SemVer major release.

Ideally this can be fixed in a patch release ASAP?

@stephybun
Copy link
Member Author

Thanks for pointing this out @stevehipwell. I've opened #22284 to rearrange the validation which should go into the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants