Repository: cloudtools/troposphere

Issue:
Tags types seems a bit confusing for newcomers
Hello,

Looking at the current [CloudFormation resource provider schemas](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/resource-type-schemas.html), tags properties are mainly expressed in 2 different types (`array` or `object`):

AWS::EC2::Instance:
```json
"Tags" : {
  "uniqueItems" : false,
  "description" : "The tags to add to the instance.",
  "insertionOrder" : false,
  "type" : "array",
  "items" : {
    "$ref" : "#/definitions/Tag"
  }
},
```

AWS::Batch::JobDefinition:
```json
"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)?

Discussion:
@markpeek: 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: 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](https://github.com/cloudtools/troposphere/blob/c6df0662e19aa2fdd82e18c8cd02ff6fb9aacf0a/troposphere/batch.py#L658)).
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: 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: Hello @markpeek,

I have just tested your branch with my originally failing code and now troposphere reports the error :slightly_smiling_face::
```console
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 :+1:.@markpeek: @gautaz thank you for the issue and for validating the patch. Always great to have fresh eyes on pain points like this one.@gautaz: You're welcome, troposphere is amazing!

Solution patch:
diff --git a/troposphere/__init__.py b/troposphere/__init__.py
index e618ae70..d4e78ed9 100644
--- a/troposphere/__init__.py
+++ b/troposphere/__init__.py
@@ -252,9 +252,11 @@ class BaseAWSObject:
             expected_type = self.props[name][0]
 
             # If the value is a AWSHelperFn we can't do much validation
-            # we'll have to leave that to Amazon.  Maybe there's another way
+            # we'll have to leave that to Amazon. Maybe there's another way
             # to deal with this that we'll come up with eventually
-            if isinstance(value, AWSHelperFn):
+            #
+            # Don't do this for Tags since we can validate the assigned type below
+            if isinstance(value, AWSHelperFn) and name != "Tags":
                 return self.properties.__setitem__(name, value)
 
             # If it's a function, call it...


Test patch:
diff --git a/tests/test_serverless.py b/tests/test_serverless.py
index 2db1ab1c..fe7e976a 100644
--- a/tests/test_serverless.py
+++ b/tests/test_serverless.py
@@ -1,6 +1,6 @@
 import unittest
 
-from troposphere import ImportValue, Parameter, Ref, Sub, Tags, Template
+from troposphere import ImportValue, Parameter, Ref, Sub, Template
 from troposphere.s3 import Filter, Rules, S3Key
 from troposphere.serverless import (
     SERVERLESS_TRANSFORM,
@@ -70,7 +70,7 @@ class TestServerless(unittest.TestCase):
             Handler="index.handler",
             Runtime="nodejs",
             CodeUri="s3://bucket/handler.zip",
-            Tags=Tags({"Tag1": "TagValue1", "Tag2": "TagValue2"}),
+            Tags={"Tag1": "TagValue1", "Tag2": "TagValue2"},
         )
         t = Template()
         t.add_resource(serverless_func)
diff --git a/tests/test_tags.py b/tests/test_tags.py
index be5886b6..b3cb4ebf 100644
--- a/tests/test_tags.py
+++ b/tests/test_tags.py
@@ -73,6 +73,47 @@ class TestTags(unittest.TestCase):
         with self.assertRaises(TypeError):
             Tags({}, "key", "value")
 
+    def test_json_tags(self):
+        from troposphere.batch import ContainerProperties, JobDefinition
+
+        JobDefinition(
+            "myjobdef",
+            Type="container",
+            ContainerProperties=ContainerProperties(
+                Image="image",
+                Vcpus=2,
+                Memory=2000,
+                Command=["echo", "foobar"],
+            ),
+            Tags={"foo": "bar"},
+        )
+
+        with self.assertRaises(TypeError):
+            JobDefinition(
+                "myjobdef",
+                Type="container",
+                ContainerProperties=ContainerProperties(
+                    Image="image",
+                    Vcpus=2,
+                    Memory=2000,
+                    Command=["echo", "foobar"],
+                ),
+                Tags=Tags({"foo": "bar"}),
+            )
+
+        with self.assertRaises(TypeError):
+            JobDefinition(
+                "myjobdef",
+                Type="container",
+                ContainerProperties=ContainerProperties(
+                    Image="image",
+                    Vcpus=2,
+                    Memory=2000,
+                    Command=["echo", "foobar"],
+                ),
+                Tags="string",
+            )
+
 
 if __name__ == "__main__":
     unittest.main()