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

docs/provider: Add Adding Resource Tagging Support section #10313

Merged
merged 2 commits into from
Oct 25, 2019

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Sep 30, 2019

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Release note for CHANGELOG:

NONE

Output from acceptance testing: N/A

Includes information how to work with the new internal/keyvaluetags generators.

Includes information how to work with the new `internal/keyvaluetags` generators.
@bflad bflad requested a review from a team September 30, 2019 20:38
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Sep 30, 2019
@bflad bflad added documentation Introduces or discusses updates to documentation. provider Pertains to the provider itself, rather than any interaction with AWS. labels Sep 30, 2019
@ewbankkit
Copy link
Contributor

Any guidance as to when the tag support code changes should be made for new services?

  • In the same PR as the new service client changes
  • A separate PR after the new service client PR
  • With the first new resource that supports resource tags

@bflad
Copy link
Contributor Author

bflad commented Oct 3, 2019

Hey @ewbankkit, great question. I'm not sure if the maintainers will have too strong of an opinion on this quite yet since working with the library is still very new.

For new services, it seems perfectly fine to include the tag support in the same PR as adding the dependency and service client, if you notice tagging support in the API. Maybe we can add that as a line item to the new service checklist, although it does add some slight complexity there in documenting it (since tagging support in the API may be available from the beginning or much later on, like EKS). Otherwise, I think it might be okay if we accept it separately as well.

If we run into more complicated merging situations in the library, the pull request review for new services feels too heavy with tagging support, or the process doesn't feel right as we go on, we can certainly come up with some more pragmatic guidelines. 😄

@ewbankkit
Copy link
Contributor

@bflad Agree that we should work with this new mechanism for a while before forming string opinions.

.github/CONTRIBUTING.md Outdated Show resolved Hide resolved
@ewbankkit
Copy link
Contributor

Do we want to explicitly call out what to do for resource that don't (yet) support tag-on-create, where you have to effectively call the Update functionality from the Create function?
For aws_api_gateway_v2_api I went with code like:

	if v := d.Get("tags").(map[string]interface{}); len(v) > 0 {
		arn := ...
		if err := keyvaluetags.Apigatewayv2UpdateTags(conn, arn, map[string]interface{}{}, v); err != nil {
			return fmt.Errorf("error creating tags: %s", err)
		}
	}

after having successfully created the resource and set its ID.

… show tagging on creation versus not, fix typo

Reference: #10313
@bflad
Copy link
Contributor Author

bflad commented Oct 6, 2019

Addressed the last comment about tagging on creation vs not in a732852

@ewbankkit FYI you can simplify the UpdateTags call with nil instead of map[string]interface{}{} to increase the readability slightly.

@bflad bflad added this to the v2.34.0 milestone Oct 25, 2019
@bflad bflad merged commit ef3fddc into master Oct 25, 2019
@bflad bflad deleted the d-provider-tagging branch October 25, 2019 17:12
@ghost
Copy link

ghost commented Mar 29, 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. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. provider Pertains to the provider itself, rather than any interaction with AWS. size/L Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants