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

Add tags to cluster node pools #5931

Merged
merged 8 commits into from
Mar 10, 2020

Conversation

wyarde
Copy link
Contributor

@wyarde wyarde commented Feb 27, 2020

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

  • Implemented tags for nodepools resource and datasource
  • Extended some of the tests
  • Updated documentation

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 ran go 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 of Web[H]ookProfiles in azurerm/internal/services/monitor/resource_arm_monitor_metric_alert.go. Is it OK to piggy-back this fix?
  • I tried to run acceptance tests, but even running just the containerservice ones takes 1+ hour. What is the recommended approach to validate and test?

@ghost ghost added the size/XXL label Feb 27, 2020
@wyarde wyarde force-pushed the add_tags_to_cluster_node_pools branch from 601b1ca to 5487508 Compare February 28, 2020 08:26
@mbfrahry
Copy link
Member

mbfrahry commented Mar 5, 2020

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.

@mbfrahry mbfrahry self-assigned this Mar 5, 2020
@wyarde
Copy link
Contributor Author

wyarde commented Mar 5, 2020

@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.

@wyarde
Copy link
Contributor Author

wyarde commented Mar 7, 2020

@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

@ghost ghost added size/S and removed size/XL labels Mar 10, 2020
Copy link
Collaborator

@katbyte katbyte left a 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 ✅

@katbyte katbyte added this to the v2.1.0 milestone Mar 10, 2020
@katbyte katbyte merged commit 55eb723 into hashicorp:master Mar 10, 2020
katbyte added a commit that referenced this pull request Mar 10, 2020
@ghost
Copy link

ghost commented Mar 11, 2020

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 ...

@wyarde wyarde deleted the add_tags_to_cluster_node_pools branch March 12, 2020 10:46
@ghost
Copy link

ghost commented Apr 9, 2020

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!

@ghost ghost locked and limited conversation to collaborators Apr 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants