-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Add tags to cluster node pools #5931
Add tags to cluster node pools #5931
Conversation
601b1ca
to
5487508
Compare
Hey @wyarde! Thanks for opening this PR! I noticed that the tags attribute is in all of the various places you're adding to in the version of the sdk we're using. Do some of them not work for you? Not upgrading the sdk would make getting this change in much easier. |
5487508
to
fbb9f7f
Compare
@mbfrahry Thank you for looking into this. That's a great point! I missed the nuance between API version and SDK version. Indeed, I 'just' need an API version bump which fortunately drastically reduces the size of this PR 💃 . Let me know your thoughts. |
@mbfrahry Just to clarify further - The current containerservice API does not support tags on nodepools. So although sdk upgrade is not needed (see my reply above), I do still need to update the API version |
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.
Hey @wyarde, i hope you don't mind but i've merged master to get this ready to merge 🙂 LGTM now ✅
This has been released in version 2.1.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example: provider "azurerm" {
version = "~> 2.1.0"
}
# ... other configuration ... |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks! |
I'm excited to present my first attempt to contribute back to Terraform. Thanks in advance for guiding me to get this PR in good shape to get merged.
Goal of PR
Add tags to AKS nodepools
Changes
Attention points
Although the change itself is small, this PR ended up touching almost 1K files because of the second commit where I have added the new containerservice sdk to modules.txt (required for nodepool tags), then rango mod vendor
. Besides the new files I expected, it updated many files. Is this expected behavior? Should I approach this differently?I had to resolve compiler errors due to casing ofWeb[H]ookProfiles
inazurerm/internal/services/monitor/resource_arm_monitor_metric_alert.go
. Is it OK to piggy-back this fix?