-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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_*
- Upgrade to go-azure-sdk
#18705
Conversation
5bebf31
to
13859f9
Compare
azurerm_kubernetes_*
- Upgrade to go-azure-sdkazurerm_kubernetes_*
- Upgrade to go-azure-sdk
46ff11e
to
182b75f
Compare
182b75f
to
a20f347
Compare
376629c
to
fd441c7
Compare
ApiServerAccessProfile is not returned by default anymore
fd441c7
to
e70a8fd
Compare
@@ -910,6 +912,7 @@ func resourceKubernetesCluster() *pluginsdk.Resource { | |||
Type: pluginsdk.TypeBool, | |||
Optional: true, | |||
ForceNew: true, | |||
Default: false, |
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.
@tombuildsstuff ApiServerAccessProfile
is not returned when having 'default' settings anymore, which means I'm setting values by default now below. Does this mean I should add Default: false
to the schema (like done)? Does this change requires a state migration?
Or is there a simpler solution?
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.
Taking a look through here this should be fine (but unnecessary) since this implicitly defaults to false
in the read function?
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.
Indeed, it implicitly defaults to 'false' in the read function, which is a change. I made it here explicit.
The main question is whether this defaulting (implicit or explicit) is a breaking change which requires a state migration, as this is also a ForceNew
setting.
@aristosvo what's the best practice, should I wait till you merge ? or should I create a new PR based on your PR to add missing features that the SDK brings in ? |
I think HC wants to merge this one first anyway (and it might still need some small changes to comply). But that shouldn't stop you from working on it. I'm not sure how good your rebase skills are, but a PR based on this work should be possible with a relatively simple rebase after merging. Btw, I expect #18667 to be merged first anyway, as that one is already being reviewed atm. |
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.
The tests look mostly fine, there is one new failure which looks like it might have coincided with the dalek running. We triggered another round of tests, once those are done and the merge conflict is resolved this should be good to go!
d4cf72b
to
3c41d8f
Compare
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 @aristosvo LGTM 🥮
This functionality has been released in v3.29.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
PR to migrate to go-azure-sdk for
azurerm_kubernetes_*
Status: compiling, run all tests, rebased
Next steps: Review time!
Acceptance tests
Most passing, leftovers seem random failures except on
MaintenanceConfig
testsAnd fixed:
and a different error showing up:
And fixed: