-
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_cluster
: Support for CSI arguments
#19396
Conversation
@CorrenSoft I can confirm that this PR would resolve #19235. My suggestion would be to use blocks and to potentially use a primary Also is there a reason why you're using |
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 for this PR @CorrenSoft, this is off to a great start!
As suggested by @stevehipwell, I think these would benefit from being grouped into a block called storage_profile
. I also left a few more comments in-line regarding the property names and the property enabled
which we don't typically expose in Terraform.
Other than that as you've mentioned the docs need to be updated, and we also need to add some tests around this.
@stevehipwell, I've used V1 because that was the value that I got when I read it from an existing cluster, and also because in any reference that I could find about the valid values they were in uppercase. @stephybun, thanks for the feedback :)
|
@CorrenSoft that's correct 🙂 |
@CorrenSoft I'd suggest that |
Co-authored-by: stephybun <[email protected]>
Co-authored-by: stephybun <[email protected]>
Co-authored-by: stephybun <[email protected]>
@stevehipwell I changed the values to @stephybun I completed the refactor, updated the docs and added a new test (creates cluster with values different from default). Let me know if anything else is needed. Note: Still running the Acc Tests. |
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 for making those changes @CorrenSoft and for touching up the docs! The tests are currently running but I don't expect any issues, aside from the few comments left in-line I have one more question which is, are these properties actually updateable?
@stevehipwell the convention in the AzureRM provider is to use whatever casing is specified in the API spec/enums provided by Azure. Since there are none for these and the API seems to accept both upper and lower casing I would agree with you that we should go with the industry wide convention for representing versions. As an aside in the swagger they used lower case v1 in the description. |
They used lowercase in the docs too but we've all been caught out by mixed case sensitivity in Azure. |
Yes, all of them. The only catch is if you try to use the |
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.
Great work on this, thanks @CorrenSoft. LGTM 💯
This functionality has been released in v3.35.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. |
May solve #19235
Added 3 optional properties and an optional block (because the disk driver has an additional parameter to specify the version).
Example of implementation:
Considering the SDK's Object Model, I have two questions on my mind, and I would like to have some feedback:
1- Should all be blocks instead of properties, to allow easy expansion if in the future extra fields are added?
2- Should be all this encapsulated in a single block instead of individual components?
Note: The docs are yet to be updated, will do so upon receiving feedback on the code.