Repository: aws/aws-cli
Pull request: Support for packaging GlueJob resources
Fixes #3570 

The AWS::Glue::Job resource has a ScriptLocation property under a Command property.  This adds support for packaging that file. 

In order to support this the base Resource property was updated to support nested properties.



By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.


Problem statement:
Add `cloudformation package` support for AWS::Glue::Job resource type
Instead of having to upload Glue scripts to S3 manually, it would be nice if it could be done with `aws cloudformation package` command the same way it's done for `AWS::CloudFormation::Stack` or `AWS::Lambda::Function` resource types:

```yaml
Resources:
  MyGlueJob:
    Type: AWS::Glue::Job
    Properties:
      Name: MyGlueJob
      Command:
        ScriptLocation: ./gluescript.scala
        Name: glueetl
        ...
```

Hints from issue discussion:
@ilyasotkov Thanks for reaching out.  This seems like it would be very helpful.  I have changed the label to feature request. @sanathkr what are your thoughts on this idea?
What are the current workarounds ?
Kill me but i cant find how to provide inline code for  glue in cloudformation template. I refuse to believe that the only option is to actually create bucket manually, then upload the script, and then hopping that i get the path right paste it into the cloudformation template. 
We should implement this. It should be a few lines of code change and some tests change. Could be a good starter project for someone. PRs welcome!
My mates need this feature and I gave it a 5 minutes investigation, which I'll share here.

The relevant bit is [artifact_exporter.py](https://github.com/aws/aws-cli/blob/develop/awscli/customizations/cloudformation/artifact_exporter.py). I thought about creating a new subclass of `Resource` just like `ServerlessFunctionResource` but then I realised that the properties of all supported resources are top level, while with glue script we need to one level down: `AWS::Glue::Job.JobCommand.ScriptLocation`. It might be trivial but I don't think this is currently supported by the [`Resource`](https://github.com/aws/aws-cli/blob/3251a4b633ee2e09b11463871fa27e0139011864/awscli/customizations/cloudformation/artifact_exporter.py#L216) class.

Happy to be proven wrong of course!
@sanathkr and @andreabedini - Thanks for your feedback.  I added the 'needs-contributors' label. 
This came up at with some teams at my company today.  I'm looking into implementing this.  Here's what I'm proposing as an implementation.  Please provide any feedback.  I'll open a PR once this is implemented.

As @andreabedini mentioned, the relevant functionality is in [artifact_exporter.py](customizations/cloudformation/artifact_exporter.py).  And the complexity is introduced by the ScriptLocation being nested under the Location property of the AWS::Glue::Job resource.

Add a new GlueCommandJobCommandScriptLocationResource.
```
class GlueCommandJobCommandScriptLocationResource(Resource):
    RESOURCE_TYPE = "AWS::Glue::Job"
    # Note the PROPERTY_NAME includes a '.' implying it's nested.
    PROPERTY_NAME = "Command.ScriptLocation"
```

In order to support the nested property (ie Command.ScriptLocation), we will need to replace all of the direct resource_dict accessors, with 2 new get_nested_property_value and set_nested_property_value methods that will split the PROPERTY_NAME on '.' and traverse the resource_dict to find the appropriate object.

```
def get_nested_property_value(resource_dict, property_name):
    """
    Searches the resource_dict for nested properties by splitting the property_name
    on '.'

    :param resource_dict:   Dictionary containing resource definition
    :param property_name:   Property name of CloudFormation resource where this
                            local path is present

    :return:                Value of the property
    """

    # Support nested properties by allowing '.' in the PROPERTY_NAME
    if '.' in property_name:
        sub_property_names = property_name.split('.')
        property_value = resource_dict.get(sub_property_names[0], None)
        for sub_property_name in sub_property_names[1:]:
            if property_value:
                property_value = property_value.get(sub_property_name, None)
    else:
        property_value = resource_dict.get(property_name, None)

    return property_value


def set_nested_property_value(resource_dict, property_name, new_value):
    """
    Searches the resource_dict for nested properties by splitting the property_name
    on '.' and sets the nested property to new_value.


    :param resource_dict:   Dictionary containing resource definition
    :param property_name:   Property name of CloudFormation resource where this
                            local path is present
    :param new_value:       The new value for the property
    :raise:                 KeyError if a property isn't found in the dictionary
    """
    # Support nested properties by allowing '.' in the PROPERTY_NAME
    # Assumes that the property exists in the dictionary
    if '.' in property_name:
        sub_property_names = property_name.split('.')
        property_dict = resource_dict[sub_property_names[0]]
        for sub_property_name in sub_property_names[1:-1]:
            property_dict = property_dict[sub_property_name]
        property_dict[sub_property_names[-1]] = new_value
    else:
        resource_dict[property_name] = new_value
```
This would b such a great feature to have . I am currently struggling setting cicd for glue and this will help us do that. Thanks so much.
@justnance @sanathkr is there anything I need to do to get #4019 reviewed and merged?

Commit message: Implement recursion in nested property discovery

Patch to review:
diff --git a/awscli/customizations/cloudformation/artifact_exporter.py b/awscli/customizations/cloudformation/artifact_exporter.py
index cba80c609aef..c338cb537388 100644
--- a/awscli/customizations/cloudformation/artifact_exporter.py
+++ b/awscli/customizations/cloudformation/artifact_exporter.py
@@ -94,6 +94,51 @@ def parse_s3_url(url,
                      "{0}".format(url))
 
 
+def get_nested_property_value(resource_dict, property_name):
+    """
+    Searches the resource_dict for nested properties by splitting the property_name
+    on '.'
+
+    :param resource_dict:   Dictionary containing resource definition
+    :param property_name:   Property name of CloudFormation resource where this
+                            local path is present
+
+    :return:                Value of the property
+    """
+
+    if '.' not in property_name:
+        return resource_dict.get(property_name, None)
+    
+    property_names = property_name.split('.', 1)
+    # default to empty dictionary so recursion has something to operate on
+    sub_property_dict = resource_dict.get(property_names[0], {}) 
+    return get_nested_property_value(sub_property_dict, property_names[1])
+
+
+def set_nested_property_value(resource_dict, property_name, new_value):
+    """
+    Searches the resource_dict for nested properties by splitting the property_name
+    on '.' and sets the nested property to new_value.
+
+
+    :param resource_dict:   Dictionary containing resource definition
+    :param property_name:   Property name of CloudFormation resource where this
+                            local path is present
+    :param new_value:       The new value for the property
+    :raise:                 KeyError if a property isn't found in the dictionary
+    """
+    # Support nested properties by allowing '.' in the PROPERTY_NAME
+    # Assumes that the property exists in the dictionary
+
+    if '.' not in property_name:
+        resource_dict[property_name] = new_value
+        return
+    
+    property_names = property_name.split('.', 1)
+    property_dict = resource_dict[property_names[0]]
+    return set_nested_property_value(property_dict, property_names[1], new_value)
+
+
 def upload_local_artifacts(resource_id, resource_dict, property_name,
                            parent_dir, uploader):
     """
@@ -120,7 +165,7 @@ def upload_local_artifacts(resource_id, resource_dict, property_name,
     :raise:                 ValueError if path is not a S3 URL or a local path
     """
 
-    local_path = resource_dict.get(property_name, None)
+    local_path = get_nested_property_value(resource_dict, property_name)
 
     if local_path is None:
         # Build the root directory and upload to S3
@@ -232,7 +277,7 @@ def export(self, resource_id, resource_dict, parent_dir):
         if resource_dict is None:
             return
 
-        property_value = resource_dict.get(self.PROPERTY_NAME, None)
+        property_value = get_nested_property_value(resource_dict, self.PROPERTY_NAME)
 
         if not property_value and not self.PACKAGE_NULL_PROPERTY:
             return
@@ -248,7 +293,7 @@ def export(self, resource_id, resource_dict, parent_dir):
         if is_local_file(property_value) and not \
                 is_zip_file(property_value) and self.FORCE_ZIP:
             temp_dir = copy_to_temp_dir(property_value)
-            resource_dict[self.PROPERTY_NAME] = temp_dir
+            set_nested_property_value(resource_dict, self.PROPERTY_NAME, temp_dir)
 
         try:
             self.do_export(resource_id, resource_dict, parent_dir)
@@ -269,10 +314,10 @@ def do_export(self, resource_id, resource_dict, parent_dir):
         Default export action is to upload artifacts and set the property to
         S3 URL of the uploaded object
         """
-        resource_dict[self.PROPERTY_NAME] = \
-            upload_local_artifacts(resource_id, resource_dict,
+        uploaded_url = upload_local_artifacts(resource_id, resource_dict,
                                    self.PROPERTY_NAME,
                                    parent_dir, self.uploader)
+        set_nested_property_value(resource_dict, self.PROPERTY_NAME, uploaded_url)
 
 
 class ResourceWithS3UrlDict(Resource):
@@ -299,11 +344,12 @@ def do_export(self, resource_id, resource_dict, parent_dir):
                                    self.PROPERTY_NAME,
                                    parent_dir, self.uploader)
 
-        resource_dict[self.PROPERTY_NAME] = parse_s3_url(
+        parsed_url = parse_s3_url(
                 artifact_s3_url,
                 bucket_name_property=self.BUCKET_NAME_PROPERTY,
                 object_key_property=self.OBJECT_KEY_PROPERTY,
                 version_property=self.VERSION_PROPERTY)
+        set_nested_property_value(resource_dict, self.PROPERTY_NAME, parsed_url)
 
 
 class ServerlessFunctionResource(Resource):
@@ -459,8 +505,9 @@ def do_export(self, resource_id, resource_dict, parent_dir):
 
             # TemplateUrl property requires S3 URL to be in path-style format
             parts = parse_s3_url(url, version_property="Version")
-            resource_dict[self.PROPERTY_NAME] = self.uploader.to_path_style_s3_url(
+            s3_path_url = self.uploader.to_path_style_s3_url(
                     parts["Key"], parts.get("Version", None))
+            set_nested_property_value(resource_dict, self.PROPERTY_NAME, s3_path_url)
 
 
 class ServerlessApplicationResource(CloudFormationStackResource):
@@ -472,6 +519,16 @@ class ServerlessApplicationResource(CloudFormationStackResource):
     PROPERTY_NAME = "Location"
 
 
+
+class GlueJobCommandScriptLocationResource(Resource):
+    """
+    Represents Glue::Job resource.
+    """
+    RESOURCE_TYPE = "AWS::Glue::Job"
+    # Note the PROPERTY_NAME includes a '.' implying it's nested.
+    PROPERTY_NAME = "Command.ScriptLocation"
+
+
 RESOURCES_EXPORT_LIST = [
     ServerlessFunctionResource,
     ServerlessApiResource,
@@ -487,6 +544,7 @@ class ServerlessApplicationResource(CloudFormationStackResource):
     ServerlessApplicationResource,
     ServerlessLayerVersionResource,
     LambdaLayerVersionResource,
+    GlueJobCommandScriptLocationResource,
 ]
 
 METADATA_EXPORT_LIST = [
diff --git a/awscli/examples/cloudformation/_package_description.rst b/awscli/examples/cloudformation/_package_description.rst
index 1642f204d980..88ff2fe9d8dd 100644
--- a/awscli/examples/cloudformation/_package_description.rst
+++ b/awscli/examples/cloudformation/_package_description.rst
@@ -21,6 +21,7 @@ This command can upload local artifacts referenced in the following places:
     - ``Location`` parameter for the ``AWS::Include`` transform
     - ``SourceBundle`` property for the ``AWS::ElasticBeanstalk::ApplicationVersion`` resource
     - ``TemplateURL`` property for the ``AWS::CloudFormation::Stack`` resource
+    - ``Command.ScriptLocation`` property for the ``AWS::Glue::Job`` resource
 
 
 To specify a local artifact in your template, specify a path to a local file or folder,
diff --git a/tests/unit/customizations/cloudformation/test_artifact_exporter.py b/tests/unit/customizations/cloudformation/test_artifact_exporter.py
index 8964b6c97056..5963991deb7b 100644
--- a/tests/unit/customizations/cloudformation/test_artifact_exporter.py
+++ b/tests/unit/customizations/cloudformation/test_artifact_exporter.py
@@ -15,6 +15,7 @@
 from awscli.customizations.cloudformation.artifact_exporter \
     import is_s3_url, parse_s3_url, is_local_file, is_local_folder, \
     upload_local_artifacts, zip_folder, make_abs_path, make_zip, \
+    get_nested_property_value, set_nested_property_value, \
     Template, Resource, ResourceWithS3UrlDict, ServerlessApiResource, \
     ServerlessFunctionResource, GraphQLSchemaResource, \
     LambdaFunctionResource, ApiGatewayRestApiResource, \
@@ -26,7 +27,8 @@
     AppSyncResolverRequestTemplateResource, \
     AppSyncResolverResponseTemplateResource, \
     AppSyncFunctionConfigurationRequestTemplateResource, \
-    AppSyncFunctionConfigurationResponseTemplateResource
+    AppSyncFunctionConfigurationResponseTemplateResource, \
+    GlueJobCommandScriptLocationResource
 
 
 def test_is_s3_url():
@@ -143,6 +145,12 @@ def test_all_resources_export():
         {
             "class": ServerlessRepoApplicationLicense,
             "expected_result": uploaded_s3_url
+        },
+        {
+            "class": GlueJobCommandScriptLocationResource,
+            "expected_result": {
+                    "ScriptLocation": uploaded_s3_url 
+            }
         }
     ]
 
@@ -162,9 +170,22 @@ def _helper_verify_export_resources(
     upload_local_artifacts_mock.reset_mock()
 
     resource_id = "id"
-    resource_dict = {
-        test_class.PROPERTY_NAME: "foo"
-    }
+
+    if '.' in test_class.PROPERTY_NAME:
+        reversed_property_names = test_class.PROPERTY_NAME.split('.')
+        reversed_property_names.reverse()
+        property_dict = {
+            reversed_property_names[0]: "foo"
+        }
+        for sub_property_name in reversed_property_names[1:]:
+            property_dict = {
+                sub_property_name: property_dict
+            }
+        resource_dict = property_dict
+    else:
+        resource_dict = {
+            test_class.PROPERTY_NAME: "foo"
+        }
     parent_dir = "dir"
 
     upload_local_artifacts_mock.return_value = uploaded_s3_url
@@ -178,7 +199,11 @@ def _helper_verify_export_resources(
                                                         test_class.PROPERTY_NAME,
                                                         parent_dir,
                                                         s3_uploader_mock)
-    result = resource_dict[test_class.PROPERTY_NAME]
+    if '.' in test_class.PROPERTY_NAME:
+        top_level_property_name = test_class.PROPERTY_NAME.split('.')[0]
+        result = resource_dict[top_level_property_name]
+    else:
+        result = resource_dict[test_class.PROPERTY_NAME]
     assert_equal(result, expected_result)
 
 
@@ -243,6 +268,108 @@ def test_is_local_folder(self):
             self.assertTrue(is_local_folder(filename))
             self.assertFalse(is_local_file(filename))
 
+    def test_get_nested_property_value_1levels(self):
+        resource_dict = { 
+            'Foo': 'testfile.yaml'
+        }
+        property_value = get_nested_property_value(resource_dict, 'Foo')
+        self.assertEquals(property_value, 'testfile.yaml')
+
+    def test_get_nested_property_value_1level_missing_returns_None(self):
+        resource_dict = {}
+        property_value = get_nested_property_value(resource_dict, 'Foo')
+        self.assertEquals(property_value, None)
+
+    def test_get_nested_property_value_2levels(self):
+        resource_dict = { 
+            'Foo': {
+                'Bar': 'testfile.yaml'
+            }
+        }
+        property_value = get_nested_property_value(resource_dict, 'Foo.Bar')
+        self.assertEquals(property_value, 'testfile.yaml')
+
+    def test_get_nested_property_value_2levels_missing_returns_None(self):
+        resource_dict = { 
+            'Foo': {
+                'Bar': 'testfile.yaml'
+            }
+        }
+        property_value = get_nested_property_value(resource_dict, 'other.other.other')
+        self.assertEquals(property_value, None)
+
+    def test_get_nested_property_value_multiplelevels(self):
+        resource_dict = { 
+            'Foo': {
+                'Bar': {
+                    'Fizz': {
+                        'Buzz': 'testfile.yaml'
+                    }
+                }
+            }
+        }
+        property_value = get_nested_property_value(resource_dict, 'Foo.Bar.Fizz.Buzz')
+        self.assertEquals(property_value, 'testfile.yaml')
+
+    def test_get_nested_property_value_multiplelevels_missing_returns_None(self):
+        resource_dict = { 
+            'Foo': {
+                'Bar': {
+                    'Fizz': {
+                        'Buzz': 'testfile.yaml'
+                    }
+                }
+            }
+        }
+        property_value = get_nested_property_value(resource_dict, 'Foo.Bar.Fizz.other')
+        self.assertEquals(property_value, None)
+
+    def test_set_nested_property_value_1levels(self):
+        resource_dict = { 
+            'Foo': 'testfile.yaml'
+        }
+        set_nested_property_value(resource_dict, 'Foo', 'otherfile.yaml')
+        expected_resource_dict = { 
+            'Foo': 'otherfile.yaml'
+        }
+        self.assertEquals(resource_dict, expected_resource_dict)
+
+    def test_set_nested_property_value_2levels(self):
+        resource_dict = { 
+            'Foo': {
+                'Bar': 'testfile.yaml'
+            }
+        }
+        set_nested_property_value(resource_dict, 'Foo.Bar', 'otherfile.yaml')
+        expected_resource_dict = { 
+            'Foo': {
+                'Bar': 'otherfile.yaml'
+            }
+        }
+        self.assertEquals(resource_dict, expected_resource_dict)
+
+    def test_set_nested_property_value_multiplelevels(self):
+        resource_dict = { 
+            'Foo': {
+                'Bar': {
+                    'Fizz': {
+                        'Buzz': 'testfile.yaml'
+                    }
+                }
+            }
+        }
+        set_nested_property_value(resource_dict, 'Foo.Bar.Fizz.Buzz', 'otherfile.yaml')
+        expected_resource_dict = { 
+            'Foo': {
+                'Bar': {
+                    'Fizz': {
+                        'Buzz': 'otherfile.yaml'
+                    }
+                }
+            }
+        }
+        self.assertEquals(resource_dict, expected_resource_dict)
+
     @patch("awscli.customizations.cloudformation.artifact_exporter.zip_and_upload")
     def test_upload_local_artifacts_local_file(self, zip_and_upload_mock):
         # Case 1: Artifact path is a relative path
@@ -298,7 +425,6 @@ def test_upload_local_artifacts_local_file_abs_path(self, zip_and_upload_mock):
             self.s3_uploader_mock.upload_with_dedup.assert_called_with(artifact_path)
             zip_and_upload_mock.assert_not_called()
 
-
     @patch("awscli.customizations.cloudformation.artifact_exporter.zip_and_upload")
     def test_upload_local_artifacts_local_folder(self, zip_and_upload_mock):
         property_name = "property"
@@ -325,8 +451,6 @@ def test_upload_local_artifacts_local_folder(self, zip_and_upload_mock):
             zip_and_upload_mock.assert_called_once_with(absolute_artifact_path,
                                                         mock.ANY)
 
-
-
     @patch("awscli.customizations.cloudformation.artifact_exporter.zip_and_upload")
     def test_upload_local_artifacts_no_path(self, zip_and_upload_mock):
         property_name = "property"
@@ -397,7 +521,6 @@ def test_upload_local_artifacts_invalid_value(self, zip_and_upload_mock):
         zip_and_upload_mock.assert_not_called()
         self.s3_uploader_mock.upload_with_dedup.assert_not_called()
 
-
     @patch("awscli.customizations.cloudformation.artifact_exporter.make_zip")
     def test_zip_folder(self, make_zip_mock):
         zip_file_name = "name.zip"
@@ -512,7 +635,6 @@ class MockResource(Resource):
         is_zipfile_mock.assert_called_once_with(original_path)
         assert_equal(resource_dict[resource.PROPERTY_NAME], s3_url)
 
-
     @patch("shutil.rmtree")
     @patch("zipfile.is_zipfile")
     @patch("awscli.customizations.cloudformation.artifact_exporter.copy_to_temp_dir")
@@ -547,7 +669,6 @@ class MockResourceNoForceZip(Resource):
         is_zipfile_mock.assert_called_once_with(original_path)
         assert_equal(resource_dict[resource.PROPERTY_NAME], s3_url)
 
-
     @patch("awscli.customizations.cloudformation.artifact_exporter.upload_local_artifacts")
     def test_resource_empty_property_value(self, upload_local_artifacts_mock):
         # Property value is empty
@@ -633,7 +754,6 @@ class MockResource(Resource):
         with self.assertRaises(exceptions.ExportFailedError):
             resource.export(resource_id, resource_dict, parent_dir)
 
-
     @patch("awscli.customizations.cloudformation.artifact_exporter.upload_local_artifacts")
     def test_resource_with_s3_url_dict(self, upload_local_artifacts_mock):
         """
@@ -674,7 +794,6 @@ class MockResource(ResourceWithS3UrlDict):
             "v": "SomeVersionNumber"
         })
 
-
     @patch("awscli.customizations.cloudformation.artifact_exporter.Template")
     def test_export_cloudformation_stack(self, TemplateMock):
         stack_resource = CloudFormationStackResource(self.s3_uploader_mock)
@@ -1056,7 +1175,6 @@ def test_include_transform_export_handler_non_include_transform(self, is_local_f
         self.s3_uploader_mock.upload_with_dedup.assert_not_called()
         self.assertEquals(handler_output, {"Name": "AWS::OtherTransform", "Parameters": {"Location": "foo.yaml"}})
 
-
     def test_template_export_path_be_folder(self):
 
         template_path = "/path/foo"
@@ -1110,7 +1228,6 @@ def test_copy_to_temp_dir(self, mkdtemp_mock, copyfile_mock):
         self.assertEqual(returned_dir, temp_dir)
         copyfile_mock.assert_called_once_with(filename, temp_dir + filename)
 
-
     @contextmanager
     def make_temp_dir(self):
         filename = tempfile.mkdtemp()


Code review comments:
diff_hunk: @@ -94,6 +94,51 @@ def parse_s3_url(url,
                      "{0}".format(url))
 
 
+def get_nested_property_value(resource_dict, property_name):
path: awscli/customizations/cloudformation/artifact_exporter.py
text: @user1:
For both the `get` and `set` methods, would it be possible to use [JMESPath](https://github.com/jmespath/jmespath.py) instead? It is a JSON query language we use in the CLI codebase. Specifically, we could replace calls to `get_nested_property_value` to: `jmespath.search(property_name, resource_dict)` and calls to `set_nested_property_value` to `botocore.utils.set_value_from_jmespath`. If we use these functions instead we should be able to cut down the amount of code we need to add and not have to worry about explicitly testing the helper functions as the new functions we would be calling are already tested. 

@author:
Sure, let me look into what that will take.  I've used jmespath with the --query output from the CLI, but never worked with the jmespath library.

@author:
@user1, I've refactored this to use the recommended methods.  Thanks for pointing that out, that significantly simplified this implementation!