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

Tags types seems a bit confusing for newcomers #2268

Closed
gautaz opened this issue Sep 26, 2024 · 6 comments · Fixed by #2270
Closed

Tags types seems a bit confusing for newcomers #2268

gautaz opened this issue Sep 26, 2024 · 6 comments · Fixed by #2270

Comments

@gautaz
Copy link

gautaz commented Sep 26, 2024

Hello,

Looking at the current CloudFormation resource provider schemas, tags properties are mainly expressed in 2 different types (array or object):

AWS::EC2::Instance:

"Tags" : {
  "uniqueItems" : false,
  "description" : "The tags to add to the instance.",
  "insertionOrder" : false,
  "type" : "array",
  "items" : {
    "$ref" : "#/definitions/Tag"
  }
},

AWS::Batch::JobDefinition:

"Tags" : {
  "type" : "object"
}

The situation is quite confusing for a newcomer, in particular regarding the Tags class provided by troposphere.
This class seems to generate the array type of tags.

At first I used an instance of the Tags class to add tags to a JobDefinition which was bound to fail, is there any way to ensure that troposphere will raise an error in this case?

Are there any best practices or recommendations regarding the usage of the Tags class?
Is using a Python dictionary to express the current object type of tags the way to go (for instance for a JobDefinition)?

@markpeek
Copy link
Member

Most of the resource Tags take the first form where you can create a Tags object and it will output the correct array of Tag. The "object" or Json Tags were introduced at some point into CloudFormation and troposphere validates those against a dict. In both cases you can start with creating a dict with the key/value pairs and, in the case of the first form use:

Tags=Tags(tagsdict)

and in the second case (such as JobDefinition) use:

Tags=tagsdict

Give this a try and let me know how it goes.

I'll also note there are autoscaling Tags that have an additional PropagateAtLaunch key.

@gautaz
Copy link
Author

gautaz commented Sep 28, 2024

Thanks @markpeek for your insights.
So to summarize, always create a dictionary of tags first and then use the class Tags when an array of (Key/Value)s is needed.
As I am also using cfn-lint, I will at least be warned when I am using the wrong type.
Using a dict for JobDefinition works as expected.

Nevertheless, I was expecting troposphere to report that I was using the wrong type during the template construction but it did not when I passed an instance of the Tags class to the JobDefinition class (which seems strange since the JobDefinition Tags field is defined to be a dict).
It constructed a template with an array of (Key/Value)s in the JobDefinition and I was only warned of my mistake when cfn-lint reported it.

Is this the expected behavior?

@markpeek
Copy link
Member

Thank you for the addition context. I just created PR #2270 which should provide better validation and trigger an error if you pass a Tags object to a Json variant of Tags. Feel free to review, test, and approve.

@gautaz
Copy link
Author

gautaz commented Sep 29, 2024

Hello @markpeek,

I have just tested your branch with my originally failing code and now troposphere reports the error 🙂:

TypeError: <class 'troposphere.batch.JobDefinition'>: JobDefinition.Tags is <class 'troposphere.Tags'>, expected <class 'dict'>

Also tested with a dict to ensure is passes with the right type and it is also OK.

Thank you for such a fast fix which is also unit tested 👍.

@markpeek
Copy link
Member

@gautaz thank you for the issue and for validating the patch. Always great to have fresh eyes on pain points like this one.

@gautaz
Copy link
Author

gautaz commented Sep 29, 2024

You're welcome, troposphere is amazing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants