Personalize tags
Closed this issue · 6 comments
Hello team,
While we are using this library to set default tags to all our Pulumi components, there are certain occasions where we want to override an automatic tag with a default value. Checking the code of the library, we realized that the place where the final tags are being set, the order between custom and default tags seems to be switched. Thant is:
File: python/pulumi_aws_tags/autotag.py
Original code: props["tags"] = tags.apply(lambda tags: {**tags, **auto_tags})
Proposal: props["tags"] = tags.apply(lambda tags: {**auto_tags, **tags})
This way, we have the default tags always set and eventually they may end up customized by the tags specified by the component.
Of course we might be missing a very good reason why this was coded that way originally, but we could not fully understand why. Would it be possible to change it? We have a PR ready to go (the change and a test) but unfortunately we cannot open it in the repo.
Thank you in advance!
Hi @okiwan, this library follows the original example by Joe Duffy. Indeed, there are two possible ways how to merge the autotags with the explicitly specified tags, each having a distinct semantics. The current semantics, following the original example, expresses an intent to ensure that the final tag values are always the ones set by register_auto_tags. The one you propose expresses an intent to set the defaults which could possibly be overridden per resource. It's also a valid use case but the current behavior is a deliberate design decision, not a mistake.
Changing the behavior would change the semantics and be a breaking change. However, we could add a new parameter to register_auto_tags specifying the merge order or priority and defaulting to the current behavior. What do you think? Let me think about it some more.
Agree on you that this would be a breaking change, you are right. If it would be possible to add a parameter with the merge strategy would be great, something like (just a quick proposal)...
def register_auto_tags(
auto_tags: Mapping[str, str],
merge_strategy: Literal['auto_tags_first', 'explicit_tags_first'] = 'auto_tags_first'
) -> None:
# ...
if merge_strategy == 'explicit_tags_first':
props["tags"] = tags.apply(lambda tags: {**auto_tags, **tags})
else:
# Default strategy, auto_tags first
props["tags"] = tags.apply(lambda tags: {**tags, **auto_tags})
...would work for you? I could open the PR. The only thing I have a doubt is regarding the test, I cannot think a way to properly patch as it is created via automation, so maybe it is a thing of running a test against another environment.
Thank you, @tlinhart !
I was also thinking about the parameter and came to conclusion that it could read just override: bool which would mean override any explicitly defined tags with the same key.
I think I can create a PR including tests during the weekend.
Maybe override_autotag_priority to give it a bit more context? But yeah, that would work too. If you can make it perfect, if not more than happy to open that PR. Thanks again @tlinhart !