From f51bcb26b25d120388da54f7dc72ba9667ad5f08 Mon Sep 17 00:00:00 2001 From: igngar Date: Wed, 6 Dec 2017 14:00:44 -0800 Subject: [PATCH 01/24] initial commit for package enhancements - bucket creation --- .../customizations/cloudformation/package.py | 46 +++++++++++++++++-- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/awscli/customizations/cloudformation/package.py b/awscli/customizations/cloudformation/package.py index f0e8acc26f3b..5dc46ca4cd3c 100644 --- a/awscli/customizations/cloudformation/package.py +++ b/awscli/customizations/cloudformation/package.py @@ -15,6 +15,10 @@ import logging import sys +#import random +#import string +import uuid + import json from botocore.client import Config @@ -57,7 +61,7 @@ class PackageCommand(BasicCommand): { 'name': 's3-bucket', - 'required': True, + 'required': False, 'help_text': ( 'The name of the S3 bucket where this command uploads' ' the artifacts that are referenced in your template.' @@ -113,18 +117,54 @@ class PackageCommand(BasicCommand): ] def _run_main(self, parsed_args, parsed_globals): + region = parsed_globals.region if parsed_globals.region else "us-east-1" s3_client = self._session.create_client( "s3", config=Config(signature_version='s3v4'), - region_name=parsed_globals.region, + region_name=region, # Changed from parsed_globals.region verify=parsed_globals.verify_ssl) template_path = parsed_args.template_file if not os.path.isfile(template_path): raise exceptions.InvalidTemplatePathError( template_path=template_path) + + if(parsed_args.s3_bucket is not None): + bucket = parsed_args.s3_bucket + if parsed_globals.region==s3_client.get_bucket_location(Bucket=bucket)["LocationConstraint"]: + print("Bucket specified it's on the same region as the deployment. Nothing to do here.") + else: + print("Not cool. Bucket it's on a different region") + + else: + print("Bucket not specified.") + sts_client = self._session.create_client( + "sts", + config=Config(signature_version='s3v4'), + verify=parsed_globals.verify_ssl + ) + bucket="sam-{region}{hash}".format( + hash=str(hash((sts_client.get_caller_identity()['UserId'].split(':')[0]))), + region=region + ) + + + print("Name of the bucket to be created: ", bucket) + #region = parsed_globals.region if parsed_globals.region else "EU" + #print(region) + if parsed_globals.region: + response = s3_client.create_bucket( + Bucket=str(bucket), + CreateBucketConfiguration={'LocationConstraint':str(region)} + ) + else: + print("we didn't add region") + response = s3_client.create_bucket( + Bucket=str(bucket) + ) + + print(response) - bucket = parsed_args.s3_bucket self.s3_uploader = S3Uploader(s3_client, bucket, From cc7794e290bb6c37ffce57f608811893df1588f6 Mon Sep 17 00:00:00 2001 From: Lessa Date: Wed, 6 Dec 2017 15:47:48 -0800 Subject: [PATCH 02/24] Added first working version - Not clean code/exceptions yet --- .../cloudformation/exceptions.py | 13 ++ .../customizations/cloudformation/package.py | 126 ++++++++++++++---- 2 files changed, 114 insertions(+), 25 deletions(-) diff --git a/awscli/customizations/cloudformation/exceptions.py b/awscli/customizations/cloudformation/exceptions.py index 3d4172794258..3baacb3f38e9 100644 --- a/awscli/customizations/cloudformation/exceptions.py +++ b/awscli/customizations/cloudformation/exceptions.py @@ -46,3 +46,16 @@ class DeployFailedError(CloudFormationCommandError): "to fetch the list of events leading up to the failure" "\n" "aws cloudformation describe-stack-events --stack-name {stack_name}") + + +class PackageFailedInvalidBucketError(CloudFormationCommandError): + fmt = "S3 Bucket is invalid or does not exist: {s3_bucket}" + + +class PackageFailedRegionMismatchError(CloudFormationCommandError): + fmt = \ + ("Failed to create package as deployment and S3 bucket region mismatch" + "\n" + "S3 Bucket region: {bucket_region}" + "\n" + "Deployment region: {deploy_region}") diff --git a/awscli/customizations/cloudformation/package.py b/awscli/customizations/cloudformation/package.py index 5dc46ca4cd3c..a11297908eff 100644 --- a/awscli/customizations/cloudformation/package.py +++ b/awscli/customizations/cloudformation/package.py @@ -22,6 +22,7 @@ import json from botocore.client import Config +from botocore.exceptions import ClientError from awscli.customizations.cloudformation.artifact_exporter import Template from awscli.customizations.cloudformation.yamlhelper import yaml_dump @@ -116,26 +117,43 @@ class PackageCommand(BasicCommand): } ] + def _does_bucket_exist(self, bucket, s3_client): + try: + s3_client.head_bucket(Bucket=bucket) + except ClientError as e: + if e.response['Error']['Code'] == 404: + return False + + return True + + def _does_deploy_region_match(self, deploy_region, s3_client): + # pass + s3_loc = s3_client.get_bucket_location(Bucket=bucket)["LocationConstraint"] + return s3_loc == deploy_region + + # ***REMOVED*** + + def _run_main(self, parsed_args, parsed_globals): region = parsed_globals.region if parsed_globals.region else "us-east-1" s3_client = self._session.create_client( "s3", config=Config(signature_version='s3v4'), - region_name=region, # Changed from parsed_globals.region + region_name=region, verify=parsed_globals.verify_ssl) template_path = parsed_args.template_file if not os.path.isfile(template_path): raise exceptions.InvalidTemplatePathError( - template_path=template_path) - + template_path=template_path) + if(parsed_args.s3_bucket is not None): bucket = parsed_args.s3_bucket - if parsed_globals.region==s3_client.get_bucket_location(Bucket=bucket)["LocationConstraint"]: - print("Bucket specified it's on the same region as the deployment. Nothing to do here.") + if parsed_globals.region == s3_client.get_bucket_location(Bucket=bucket)["LocationConstraint"]: + print( + "Bucket specified it's on the same region as the deployment. Nothing to do here.") else: print("Not cool. Bucket it's on a different region") - else: print("Bucket not specified.") sts_client = self._session.create_client( @@ -143,27 +161,85 @@ def _run_main(self, parsed_args, parsed_globals): config=Config(signature_version='s3v4'), verify=parsed_globals.verify_ssl ) - bucket="sam-{region}{hash}".format( - hash=str(hash((sts_client.get_caller_identity()['UserId'].split(':')[0]))), + bucket = "sam-{region}{account}".format( + account=str(sts_client.get_caller_identity()["Account"]), region=region ) - - - print("Name of the bucket to be created: ", bucket) - #region = parsed_globals.region if parsed_globals.region else "EU" - #print(region) - if parsed_globals.region: - response = s3_client.create_bucket( - Bucket=str(bucket), - CreateBucketConfiguration={'LocationConstraint':str(region)} - ) - else: - print("we didn't add region") - response = s3_client.create_bucket( - Bucket=str(bucket) - ) - - print(response) + #If they have already created the bucket, we need to find it out. + try: + print("Checking bucket -> ", bucket) + s3_client.head_bucket(Bucket=bucket) + except ClientError as e: + if e.response["Error"]["Code"] == "404": + print("Bucket doesn't exist") + # print("Unexpected error:", sys.exc_info()) + #region = parsed_globals.region if parsed_globals.region else "EU" + #print(region) + if parsed_globals.region: + s3_client.create_bucket( + Bucket=str(bucket), + CreateBucketConfiguration={ + 'LocationConstraint': str(region)} + ) + else: + s3_client.create_bucket( + Bucket=str(bucket) + ) + # continue + # region = parsed_globals.region if parsed_globals.region else "us-east-1" + # s3_client = self._session.create_client( + # "s3", + # config=Config(signature_version='s3v4'), + # region_name=region, + # verify=parsed_globals.verify_ssl) + + # template_path = parsed_args.template_file + # if not os.path.isfile(template_path): + # raise exceptions.InvalidTemplatePathError( + # template_path=template_path) + + # if(parsed_args.s3_bucket is not None): + # bucket = parsed_args.s3_bucket + # if not self._does_bucket_exist(bucket, s3_client): + # raise exceptions.PackageFailedInvalidBucketError( + # bucket_region=s3_loc + # ) + + # if not _does_deploy_region_match(region, s3_client): + # raise exceptions.PackageFailedRegionMismatchError( + # bucket_region=s3_loc, + # deploy_region=deploy_region + # ) + + # raise Exception("Tested ;)") + # else: + # print("Bucket not specified.") + # sts_client = self._session.create_client( + # "sts", + # config=Config(signature_version='s3v4'), + # verify=parsed_globals.verify_ssl + # ) + # bucket="sam-{region}{hash}".format( + # hash=str(hash((sts_client.get_caller_identity()['UserId'].split(':')[0]))), + # region=region + # ) + + + # print("Name of the bucket to be created: ", bucket) + # #region = parsed_globals.region if parsed_globals.region else "EU" + # #print(region) + # if parsed_globals.region: + # response = s3_client.create_bucket( + # Bucket=str(bucket), + # CreateBucketConfiguration={'LocationConstraint':str(region)} + # ) + # else: + # print("we didn't add region") + # response = s3_client.create_bucket( + # Bucket=str(bucket) + # ) + + # print(response) self.s3_uploader = S3Uploader(s3_client, From 3d399b2a78343343c96f2b58512b621e1af898e7 Mon Sep 17 00:00:00 2001 From: Lessa Date: Wed, 6 Dec 2017 15:58:44 -0800 Subject: [PATCH 03/24] Added '-' between Region and Account to make it compliant as Hashing was removed --- awscli/customizations/cloudformation/package.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/awscli/customizations/cloudformation/package.py b/awscli/customizations/cloudformation/package.py index a11297908eff..d7d8b2b17275 100644 --- a/awscli/customizations/cloudformation/package.py +++ b/awscli/customizations/cloudformation/package.py @@ -161,7 +161,7 @@ def _run_main(self, parsed_args, parsed_globals): config=Config(signature_version='s3v4'), verify=parsed_globals.verify_ssl ) - bucket = "sam-{region}{account}".format( + bucket = "sam-{region}-{account}".format( account=str(sts_client.get_caller_identity()["Account"]), region=region ) From c11c21bafaf25218b76118db20229f5a573e053f Mon Sep 17 00:00:00 2001 From: Lessa Date: Wed, 6 Dec 2017 17:25:30 -0800 Subject: [PATCH 04/24] Updated exceptions and removed initial clutter --- .../cloudformation/exceptions.py | 10 +- .../customizations/cloudformation/package.py | 93 +++---------------- 2 files changed, 15 insertions(+), 88 deletions(-) diff --git a/awscli/customizations/cloudformation/exceptions.py b/awscli/customizations/cloudformation/exceptions.py index 3baacb3f38e9..7455ac950965 100644 --- a/awscli/customizations/cloudformation/exceptions.py +++ b/awscli/customizations/cloudformation/exceptions.py @@ -48,14 +48,10 @@ class DeployFailedError(CloudFormationCommandError): "aws cloudformation describe-stack-events --stack-name {stack_name}") -class PackageFailedInvalidBucketError(CloudFormationCommandError): - fmt = "S3 Bucket is invalid or does not exist: {s3_bucket}" - - class PackageFailedRegionMismatchError(CloudFormationCommandError): fmt = \ ("Failed to create package as deployment and S3 bucket region mismatch" + "\n\n" + "\tS3 Bucket region: {bucket_region}" "\n" - "S3 Bucket region: {bucket_region}" - "\n" - "Deployment region: {deploy_region}") + "\tDeployment region: {deploy_region}") diff --git a/awscli/customizations/cloudformation/package.py b/awscli/customizations/cloudformation/package.py index d7d8b2b17275..85fadbd3636e 100644 --- a/awscli/customizations/cloudformation/package.py +++ b/awscli/customizations/cloudformation/package.py @@ -14,11 +14,6 @@ import os import logging import sys - -#import random -#import string -import uuid - import json from botocore.client import Config @@ -117,21 +112,8 @@ class PackageCommand(BasicCommand): } ] - def _does_bucket_exist(self, bucket, s3_client): - try: - s3_client.head_bucket(Bucket=bucket) - except ClientError as e: - if e.response['Error']['Code'] == 404: - return False - - return True - - def _does_deploy_region_match(self, deploy_region, s3_client): - # pass - s3_loc = s3_client.get_bucket_location(Bucket=bucket)["LocationConstraint"] - return s3_loc == deploy_region - - # ***REMOVED*** + def _does_deploy_region_match(self, bucket_region, deploy_region, s3_client): + return bucket_region == deploy_region def _run_main(self, parsed_args, parsed_globals): @@ -147,13 +129,18 @@ def _run_main(self, parsed_args, parsed_globals): raise exceptions.InvalidTemplatePathError( template_path=template_path) - if(parsed_args.s3_bucket is not None): + if (parsed_args.s3_bucket is not None): bucket = parsed_args.s3_bucket - if parsed_globals.region == s3_client.get_bucket_location(Bucket=bucket)["LocationConstraint"]: - print( - "Bucket specified it's on the same region as the deployment. Nothing to do here.") + bucket_region = s3_client.get_bucket_location( + Bucket=bucket)["LocationConstraint"] + + if self._does_deploy_region_match(bucket_region, parsed_globals.region, s3_client): + print(f"[*] Bucket {bucket} already exists in the same region") else: - print("Not cool. Bucket it's on a different region") + raise exceptions.PackageFailedRegionMismatchError( + bucket_region=bucket_region, + deploy_region=parsed_globals.region + ) else: print("Bucket not specified.") sts_client = self._session.create_client( @@ -185,62 +172,6 @@ def _run_main(self, parsed_args, parsed_globals): s3_client.create_bucket( Bucket=str(bucket) ) - # continue - # region = parsed_globals.region if parsed_globals.region else "us-east-1" - # s3_client = self._session.create_client( - # "s3", - # config=Config(signature_version='s3v4'), - # region_name=region, - # verify=parsed_globals.verify_ssl) - - # template_path = parsed_args.template_file - # if not os.path.isfile(template_path): - # raise exceptions.InvalidTemplatePathError( - # template_path=template_path) - - # if(parsed_args.s3_bucket is not None): - # bucket = parsed_args.s3_bucket - # if not self._does_bucket_exist(bucket, s3_client): - # raise exceptions.PackageFailedInvalidBucketError( - # bucket_region=s3_loc - # ) - - # if not _does_deploy_region_match(region, s3_client): - # raise exceptions.PackageFailedRegionMismatchError( - # bucket_region=s3_loc, - # deploy_region=deploy_region - # ) - - # raise Exception("Tested ;)") - # else: - # print("Bucket not specified.") - # sts_client = self._session.create_client( - # "sts", - # config=Config(signature_version='s3v4'), - # verify=parsed_globals.verify_ssl - # ) - # bucket="sam-{region}{hash}".format( - # hash=str(hash((sts_client.get_caller_identity()['UserId'].split(':')[0]))), - # region=region - # ) - - - # print("Name of the bucket to be created: ", bucket) - # #region = parsed_globals.region if parsed_globals.region else "EU" - # #print(region) - # if parsed_globals.region: - # response = s3_client.create_bucket( - # Bucket=str(bucket), - # CreateBucketConfiguration={'LocationConstraint':str(region)} - # ) - # else: - # print("we didn't add region") - # response = s3_client.create_bucket( - # Bucket=str(bucket) - # ) - - # print(response) - self.s3_uploader = S3Uploader(s3_client, bucket, From 191b3a227183cc380e9a4216eb906a0074a8f07e Mon Sep 17 00:00:00 2001 From: Lessa Date: Fri, 8 Dec 2017 22:29:19 -0800 Subject: [PATCH 05/24] Adds check on whether deployment region is equals to s3 bucket specified --- .../customizations/cloudformation/package.py | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/awscli/customizations/cloudformation/package.py b/awscli/customizations/cloudformation/package.py index d7d8b2b17275..f12101fc29a5 100644 --- a/awscli/customizations/cloudformation/package.py +++ b/awscli/customizations/cloudformation/package.py @@ -126,13 +126,10 @@ def _does_bucket_exist(self, bucket, s3_client): return True - def _does_deploy_region_match(self, deploy_region, s3_client): - # pass - s3_loc = s3_client.get_bucket_location(Bucket=bucket)["LocationConstraint"] + def _does_deploy_region_match(self, s3_bucket, deploy_region, s3_client): + s3_loc = s3_client.get_bucket_location(Bucket=s3_bucket)["LocationConstraint"] return s3_loc == deploy_region - # ***REMOVED*** - def _run_main(self, parsed_args, parsed_globals): region = parsed_globals.region if parsed_globals.region else "us-east-1" @@ -149,11 +146,16 @@ def _run_main(self, parsed_args, parsed_globals): if(parsed_args.s3_bucket is not None): bucket = parsed_args.s3_bucket - if parsed_globals.region == s3_client.get_bucket_location(Bucket=bucket)["LocationConstraint"]: - print( - "Bucket specified it's on the same region as the deployment. Nothing to do here.") - else: - print("Not cool. Bucket it's on a different region") + if not _does_deploy_region_match(bucket, region, s3_client): + raise exceptions.PackageFailedRegionMismatchError( + bucket_region=s3_loc, + deploy_region=deploy_region + ) + # if parsed_globals.region == s3_client.get_bucket_location(Bucket=bucket)["LocationConstraint"]: + # print( + # "Bucket specified it's on the same region as the deployment. Nothing to do here.") + # else: + # print("Not cool. Bucket it's on a different region") else: print("Bucket not specified.") sts_client = self._session.create_client( From 68ac314e600bf8d9b7bc84e1711aeefc249ec059 Mon Sep 17 00:00:00 2001 From: Lessa Date: Fri, 8 Dec 2017 23:00:07 -0800 Subject: [PATCH 06/24] Created up bucket creation code, removed prints and added stdout compliant message --- .../customizations/cloudformation/package.py | 103 +++++------------- 1 file changed, 26 insertions(+), 77 deletions(-) diff --git a/awscli/customizations/cloudformation/package.py b/awscli/customizations/cloudformation/package.py index f12101fc29a5..326405c589f2 100644 --- a/awscli/customizations/cloudformation/package.py +++ b/awscli/customizations/cloudformation/package.py @@ -45,6 +45,11 @@ class PackageCommand(BasicCommand): "--stack-name " "\n") + + MSG_PACKAGE_S3_BUCKET_CREATION = ( + "Bucket {bucket} doesn't exist.\n" + "Creating s3://{bucket} at {region} region.\n") + NAME = "package" DESCRIPTION = BasicCommand.FROM_FILE("cloudformation", @@ -144,20 +149,14 @@ def _run_main(self, parsed_args, parsed_globals): raise exceptions.InvalidTemplatePathError( template_path=template_path) - if(parsed_args.s3_bucket is not None): + if (parsed_args.s3_bucket is not None): bucket = parsed_args.s3_bucket if not _does_deploy_region_match(bucket, region, s3_client): raise exceptions.PackageFailedRegionMismatchError( bucket_region=s3_loc, deploy_region=deploy_region ) - # if parsed_globals.region == s3_client.get_bucket_location(Bucket=bucket)["LocationConstraint"]: - # print( - # "Bucket specified it's on the same region as the deployment. Nothing to do here.") - # else: - # print("Not cool. Bucket it's on a different region") else: - print("Bucket not specified.") sts_client = self._session.create_client( "sts", config=Config(signature_version='s3v4'), @@ -167,82 +166,32 @@ def _run_main(self, parsed_args, parsed_globals): account=str(sts_client.get_caller_identity()["Account"]), region=region ) - #If they have already created the bucket, we need to find it out. + + # Check whether SAM deployment bucket already exists otherwise create it try: - print("Checking bucket -> ", bucket) s3_client.head_bucket(Bucket=bucket) except ClientError as e: if e.response["Error"]["Code"] == "404": - print("Bucket doesn't exist") - # print("Unexpected error:", sys.exc_info()) - #region = parsed_globals.region if parsed_globals.region else "EU" - #print(region) + sys.stdout.write(self.MSG_PACKAGE_S3_BUCKET_CREATION.format( + bucket=bucket, region=region)) + + _s3_params = { + "all_regions": { + "Bucket": bucket + }, + "us_standard": { + "Bucket": bucket, + "CreateBucketConfiguration": { + "LocationConstraint": region + } + } + } + + # Create bucket in specified region or else use us-east-1 if parsed_globals.region: - s3_client.create_bucket( - Bucket=str(bucket), - CreateBucketConfiguration={ - 'LocationConstraint': str(region)} - ) + s3_client.create_bucket(**_s3_params['all_regions']) else: - s3_client.create_bucket( - Bucket=str(bucket) - ) - # continue - # region = parsed_globals.region if parsed_globals.region else "us-east-1" - # s3_client = self._session.create_client( - # "s3", - # config=Config(signature_version='s3v4'), - # region_name=region, - # verify=parsed_globals.verify_ssl) - - # template_path = parsed_args.template_file - # if not os.path.isfile(template_path): - # raise exceptions.InvalidTemplatePathError( - # template_path=template_path) - - # if(parsed_args.s3_bucket is not None): - # bucket = parsed_args.s3_bucket - # if not self._does_bucket_exist(bucket, s3_client): - # raise exceptions.PackageFailedInvalidBucketError( - # bucket_region=s3_loc - # ) - - # if not _does_deploy_region_match(region, s3_client): - # raise exceptions.PackageFailedRegionMismatchError( - # bucket_region=s3_loc, - # deploy_region=deploy_region - # ) - - # raise Exception("Tested ;)") - # else: - # print("Bucket not specified.") - # sts_client = self._session.create_client( - # "sts", - # config=Config(signature_version='s3v4'), - # verify=parsed_globals.verify_ssl - # ) - # bucket="sam-{region}{hash}".format( - # hash=str(hash((sts_client.get_caller_identity()['UserId'].split(':')[0]))), - # region=region - # ) - - - # print("Name of the bucket to be created: ", bucket) - # #region = parsed_globals.region if parsed_globals.region else "EU" - # #print(region) - # if parsed_globals.region: - # response = s3_client.create_bucket( - # Bucket=str(bucket), - # CreateBucketConfiguration={'LocationConstraint':str(region)} - # ) - # else: - # print("we didn't add region") - # response = s3_client.create_bucket( - # Bucket=str(bucket) - # ) - - # print(response) - + s3_client.create_bucket(**_s3_params['us_standard']) self.s3_uploader = S3Uploader(s3_client, bucket, From c294e59b2cb3b783fa3e852972eafe4acfb538f3 Mon Sep 17 00:00:00 2001 From: Lessa Date: Fri, 8 Dec 2017 23:01:01 -0800 Subject: [PATCH 07/24] Removed helper function to check bucket existence as it's already being captured by CLI either way --- awscli/customizations/cloudformation/package.py | 9 --------- 1 file changed, 9 deletions(-) diff --git a/awscli/customizations/cloudformation/package.py b/awscli/customizations/cloudformation/package.py index 326405c589f2..8107f4effa2e 100644 --- a/awscli/customizations/cloudformation/package.py +++ b/awscli/customizations/cloudformation/package.py @@ -122,15 +122,6 @@ class PackageCommand(BasicCommand): } ] - def _does_bucket_exist(self, bucket, s3_client): - try: - s3_client.head_bucket(Bucket=bucket) - except ClientError as e: - if e.response['Error']['Code'] == 404: - return False - - return True - def _does_deploy_region_match(self, s3_bucket, deploy_region, s3_client): s3_loc = s3_client.get_bucket_location(Bucket=s3_bucket)["LocationConstraint"] return s3_loc == deploy_region From a645a77b3ff0231e9c0bc442bc4601784ae127f9 Mon Sep 17 00:00:00 2001 From: Lessa Date: Fri, 8 Dec 2017 23:11:49 -0800 Subject: [PATCH 08/24] pep8 lint as per contrib docs --- .../customizations/cloudformation/package.py | 37 ++++++++----------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/awscli/customizations/cloudformation/package.py b/awscli/customizations/cloudformation/package.py index 8107f4effa2e..afd7e9ca5dc7 100644 --- a/awscli/customizations/cloudformation/package.py +++ b/awscli/customizations/cloudformation/package.py @@ -11,24 +11,18 @@ # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. -import os +import json import logging +import os import sys -#import random -#import string -import uuid - -import json - -from botocore.client import Config -from botocore.exceptions import ClientError - +from awscli.customizations.cloudformation import exceptions from awscli.customizations.cloudformation.artifact_exporter import Template from awscli.customizations.cloudformation.yamlhelper import yaml_dump -from awscli.customizations.cloudformation import exceptions from awscli.customizations.commands import BasicCommand from awscli.customizations.s3uploader import S3Uploader +from botocore.client import Config +from botocore.exceptions import ClientError LOG = logging.getLogger(__name__) @@ -45,7 +39,6 @@ class PackageCommand(BasicCommand): "--stack-name " "\n") - MSG_PACKAGE_S3_BUCKET_CREATION = ( "Bucket {bucket} doesn't exist.\n" "Creating s3://{bucket} at {region} region.\n") @@ -123,10 +116,10 @@ class PackageCommand(BasicCommand): ] def _does_deploy_region_match(self, s3_bucket, deploy_region, s3_client): - s3_loc = s3_client.get_bucket_location(Bucket=s3_bucket)["LocationConstraint"] + s3_loc = s3_client.get_bucket_location(Bucket=s3_bucket)[ + "LocationConstraint"] return s3_loc == deploy_region - def _run_main(self, parsed_args, parsed_globals): region = parsed_globals.region if parsed_globals.region else "us-east-1" s3_client = self._session.create_client( @@ -158,14 +151,15 @@ def _run_main(self, parsed_args, parsed_globals): region=region ) - # Check whether SAM deployment bucket already exists otherwise create it + # Check if SAM deployment bucket already exists otherwise create it try: s3_client.head_bucket(Bucket=bucket) except ClientError as e: if e.response["Error"]["Code"] == "404": - sys.stdout.write(self.MSG_PACKAGE_S3_BUCKET_CREATION.format( - bucket=bucket, region=region)) - + sys.stdout.write( + self.MSG_PACKAGE_S3_BUCKET_CREATION.format( + bucket=bucket, region=region)) + _s3_params = { "all_regions": { "Bucket": bucket @@ -200,8 +194,8 @@ def _run_main(self, parsed_args, parsed_globals): if output_file: msg = self.MSG_PACKAGED_TEMPLATE_WRITTEN.format( - output_file_name=output_file, - output_file_path=os.path.abspath(output_file)) + output_file_name=output_file, + output_file_path=os.path.abspath(output_file)) sys.stdout.write(msg) sys.stdout.flush() @@ -212,7 +206,8 @@ def _export(self, template_path, use_json): exported_template = template.export() if use_json: - exported_str = json.dumps(exported_template, indent=4, ensure_ascii=False) + exported_str = json.dumps( + exported_template, indent=4, ensure_ascii=False) else: exported_str = yaml_dump(exported_template) From 9cde45efdf2f5f792a88fed5a2c7de8c6c4e6be9 Mon Sep 17 00:00:00 2001 From: Lessa Date: Fri, 8 Dec 2017 23:13:59 -0800 Subject: [PATCH 09/24] Removed unnecessary exception as non-existent buckets exception is captured as part of CLI already --- awscli/customizations/cloudformation/exceptions.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/awscli/customizations/cloudformation/exceptions.py b/awscli/customizations/cloudformation/exceptions.py index 3baacb3f38e9..1a73bc004adc 100644 --- a/awscli/customizations/cloudformation/exceptions.py +++ b/awscli/customizations/cloudformation/exceptions.py @@ -48,10 +48,6 @@ class DeployFailedError(CloudFormationCommandError): "aws cloudformation describe-stack-events --stack-name {stack_name}") -class PackageFailedInvalidBucketError(CloudFormationCommandError): - fmt = "S3 Bucket is invalid or does not exist: {s3_bucket}" - - class PackageFailedRegionMismatchError(CloudFormationCommandError): fmt = \ ("Failed to create package as deployment and S3 bucket region mismatch" From e5e55d3ae803ff675de2bf89e69905695029a22c Mon Sep 17 00:00:00 2001 From: Lessa Date: Fri, 8 Dec 2017 23:32:05 -0800 Subject: [PATCH 10/24] Fixed NameError in nose tests - typo missing self._does_deploy_region_match --- awscli/customizations/cloudformation/package.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/awscli/customizations/cloudformation/package.py b/awscli/customizations/cloudformation/package.py index afd7e9ca5dc7..78ffcb787067 100644 --- a/awscli/customizations/cloudformation/package.py +++ b/awscli/customizations/cloudformation/package.py @@ -135,7 +135,7 @@ def _run_main(self, parsed_args, parsed_globals): if (parsed_args.s3_bucket is not None): bucket = parsed_args.s3_bucket - if not _does_deploy_region_match(bucket, region, s3_client): + if not self._does_deploy_region_match(bucket, region, s3_client): raise exceptions.PackageFailedRegionMismatchError( bucket_region=s3_loc, deploy_region=deploy_region From 671b3db06eeab4636a1736cbe85abdfeed81ecf2 Mon Sep 17 00:00:00 2001 From: Lessa Date: Thu, 14 Dec 2017 12:57:03 +0000 Subject: [PATCH 11/24] Simplified code with _get_bucket_region to make changes "testable" --- awscli/customizations/cloudformation/package.py | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/awscli/customizations/cloudformation/package.py b/awscli/customizations/cloudformation/package.py index 78ffcb787067..3117ae36f8e0 100644 --- a/awscli/customizations/cloudformation/package.py +++ b/awscli/customizations/cloudformation/package.py @@ -115,10 +115,9 @@ class PackageCommand(BasicCommand): } ] - def _does_deploy_region_match(self, s3_bucket, deploy_region, s3_client): - s3_loc = s3_client.get_bucket_location(Bucket=s3_bucket)[ - "LocationConstraint"] - return s3_loc == deploy_region + def _get_bucket_region(self, s3_bucket, s3_client): + s3_loc = s3_client.get_bucket_location(Bucket=s3_bucket) + return s3_loc.get("LocationConstraint", "us-east-1") def _run_main(self, parsed_args, parsed_globals): region = parsed_globals.region if parsed_globals.region else "us-east-1" @@ -127,18 +126,18 @@ def _run_main(self, parsed_args, parsed_globals): config=Config(signature_version='s3v4'), region_name=region, verify=parsed_globals.verify_ssl) - template_path = parsed_args.template_file if not os.path.isfile(template_path): raise exceptions.InvalidTemplatePathError( template_path=template_path) if (parsed_args.s3_bucket is not None): - bucket = parsed_args.s3_bucket - if not self._does_deploy_region_match(bucket, region, s3_client): + s3_bucket = parsed_args.s3_bucket + s3_bucket_region = self._get_bucket_region(s3_bucket, s3_client) + if not s3_bucket_region == region: raise exceptions.PackageFailedRegionMismatchError( - bucket_region=s3_loc, - deploy_region=deploy_region + bucket_region=s3_bucket_region, + deploy_region=region ) else: sts_client = self._session.create_client( From 39c37d3e516e6061b9387670c99cb65d4c37d8c5 Mon Sep 17 00:00:00 2001 From: Lessa Date: Thu, 14 Dec 2017 15:09:28 +0000 Subject: [PATCH 12/24] Tests now pass with patched code for optional s3_bucket --- .../customizations/cloudformation/package.py | 19 +++---------------- .../cloudformation/test_package.py | 5 +++++ 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/awscli/customizations/cloudformation/package.py b/awscli/customizations/cloudformation/package.py index 45857318b199..3486388e2a1c 100644 --- a/awscli/customizations/cloudformation/package.py +++ b/awscli/customizations/cloudformation/package.py @@ -15,16 +15,8 @@ import logging import os import sys -<<<<<<< HEAD -import json - -from botocore.client import Config -from botocore.exceptions import ClientError - -======= from awscli.customizations.cloudformation import exceptions ->>>>>>> lessa-clean-code from awscli.customizations.cloudformation.artifact_exporter import Template from awscli.customizations.cloudformation.yamlhelper import yaml_dump from awscli.customizations.commands import BasicCommand @@ -134,14 +126,15 @@ def _run_main(self, parsed_args, parsed_globals): config=Config(signature_version='s3v4'), region_name=region, verify=parsed_globals.verify_ssl) + template_path = parsed_args.template_file if not os.path.isfile(template_path): raise exceptions.InvalidTemplatePathError( template_path=template_path) if (parsed_args.s3_bucket is not None): - s3_bucket = parsed_args.s3_bucket - s3_bucket_region = self._get_bucket_region(s3_bucket, s3_client) + bucket = parsed_args.s3_bucket + s3_bucket_region = self._get_bucket_region(bucket, s3_client) if not s3_bucket_region == region: raise exceptions.PackageFailedRegionMismatchError( bucket_region=s3_bucket_region, @@ -183,13 +176,7 @@ def _run_main(self, parsed_args, parsed_globals): if parsed_globals.region: s3_client.create_bucket(**_s3_params['all_regions']) else: -<<<<<<< HEAD - s3_client.create_bucket( - Bucket=str(bucket) - ) -======= s3_client.create_bucket(**_s3_params['us_standard']) ->>>>>>> lessa-clean-code self.s3_uploader = S3Uploader(s3_client, bucket, diff --git a/tests/unit/customizations/cloudformation/test_package.py b/tests/unit/customizations/cloudformation/test_package.py index ad64c7edb273..d7838e80d352 100644 --- a/tests/unit/customizations/cloudformation/test_package.py +++ b/tests/unit/customizations/cloudformation/test_package.py @@ -74,6 +74,8 @@ def test_main(self, mock_yaml_dump): self.parsed_args.template_file = filename self.parsed_args.use_json=use_json + self.package_command._get_bucket_region = MagicMock(return_value="us-east-1") + rc = self.package_command._run_main(self.parsed_args, self.parsed_globals) self.assertEquals(rc, 0) @@ -96,6 +98,9 @@ def test_main_error(self): filename = handle.name self.parsed_args.template_file = filename + self.package_command._get_bucket_region = MagicMock( + return_value="us-east-1") + with self.assertRaises(RuntimeError): self.package_command._run_main(self.parsed_args, self.parsed_globals) From 7cca0fb4f5ce7a74ea936d12c33f19d3a6ca4b22 Mon Sep 17 00:00:00 2001 From: Lessa Date: Thu, 14 Dec 2017 15:43:13 +0000 Subject: [PATCH 13/24] Added additional tests to make optional bucket code to work; all passing now --- .../customizations/cloudformation/package.py | 8 ++- .../cloudformation/test_package.py | 57 +++++++++++++++++++ 2 files changed, 62 insertions(+), 3 deletions(-) diff --git a/awscli/customizations/cloudformation/package.py b/awscli/customizations/cloudformation/package.py index 3486388e2a1c..aeaafe1abe68 100644 --- a/awscli/customizations/cloudformation/package.py +++ b/awscli/customizations/cloudformation/package.py @@ -116,8 +116,9 @@ class PackageCommand(BasicCommand): ] def _get_bucket_region(self, s3_bucket, s3_client): - s3_loc = s3_client.get_bucket_location(Bucket=s3_bucket) - return s3_loc.get("LocationConstraint", "us-east-1") + s3_loc = s3_client.get_bucket_location( + Bucket=s3_bucket).get("LocationConstraint", "us-east-1") + return s3_loc.get def _run_main(self, parsed_args, parsed_globals): region = parsed_globals.region if parsed_globals.region else "us-east-1" @@ -146,8 +147,9 @@ def _run_main(self, parsed_args, parsed_globals): config=Config(signature_version='s3v4'), verify=parsed_globals.verify_ssl ) + account = sts_client.get_caller_identity().get('Account', "") bucket = "sam-{region}-{account}".format( - account=str(sts_client.get_caller_identity()["Account"]), + account=account, region=region ) diff --git a/tests/unit/customizations/cloudformation/test_package.py b/tests/unit/customizations/cloudformation/test_package.py index d7838e80d352..d7154a7ad9ad 100644 --- a/tests/unit/customizations/cloudformation/test_package.py +++ b/tests/unit/customizations/cloudformation/test_package.py @@ -21,6 +21,7 @@ from awscli.customizations.cloudformation.package import PackageCommand from awscli.customizations.cloudformation.artifact_exporter import Template from awscli.customizations.cloudformation.yamlhelper import yaml_dump +from awscli.customizations.cloudformation.exceptions import PackageFailedRegionMismatchError class FakeArgs(object): @@ -86,7 +87,63 @@ def test_main(self, mock_yaml_dump): self.package_command._export.reset_mock() self.package_command.write_output.reset_mock() + @patch("awscli.customizations.cloudformation.package.yaml_dump") + def test_main_without_bucket(self, mock_yaml_dump): + exported_template_str = "hello" + + self.package_command.write_output = Mock() + self.package_command._export = Mock() + mock_yaml_dump.return_value = exported_template_str + + # Create a temporary file and make this my template + with tempfile.NamedTemporaryFile() as handle: + for use_json in (False, True): + filename = handle.name + self.parsed_args.template_file = filename + self.parsed_args.use_json = use_json + + self.package_command._get_bucket_region = MagicMock( + return_value="us-east-1") + self.parsed_args.s3_bucket = None + + rc = self.package_command._run_main( + self.parsed_args, self.parsed_globals) + self.assertEquals(rc, 0) + + self.package_command._export.assert_called_once_with( + filename, use_json) + self.package_command.write_output.assert_called_once_with( + self.parsed_args.output_template_file, mock.ANY) + self.package_command._export.reset_mock() + self.package_command.write_output.reset_mock() + + @patch("awscli.customizations.cloudformation.package.yaml_dump") + def test_main_bucket_different_deployment_region(self, mock_yaml_dump): + exported_template_str = "hello" + + self.package_command.write_output = Mock() + self.package_command._export = Mock() + mock_yaml_dump.return_value = exported_template_str + + # Create a temporary file and make this my template + with tempfile.NamedTemporaryFile() as handle: + for use_json in (False, True): + filename = handle.name + self.parsed_args.template_file = filename + self.parsed_args.use_json = use_json + + self.package_command._get_bucket_region = MagicMock( + return_value="us-east-1") + self.parsed_args.s3_bucket = "bucket-in-different-region" + self.parsed_globals.region = "eu-west-1" + + with self.assertRaises(PackageFailedRegionMismatchError): + self.package_command._run_main( + self.parsed_args, self.parsed_globals) + + self.package_command._export.reset_mock() + self.package_command.write_output.reset_mock() def test_main_error(self): From 548407d8465367db39d1485c4cc044cd553a7b02 Mon Sep 17 00:00:00 2001 From: Lessa Date: Thu, 11 Jan 2018 14:49:58 +0000 Subject: [PATCH 14/24] Fixed review comment on somehow incomplete .get() --- awscli/customizations/cloudformation/package.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/awscli/customizations/cloudformation/package.py b/awscli/customizations/cloudformation/package.py index aeaafe1abe68..a0477d6658c7 100644 --- a/awscli/customizations/cloudformation/package.py +++ b/awscli/customizations/cloudformation/package.py @@ -116,9 +116,8 @@ class PackageCommand(BasicCommand): ] def _get_bucket_region(self, s3_bucket, s3_client): - s3_loc = s3_client.get_bucket_location( - Bucket=s3_bucket).get("LocationConstraint", "us-east-1") - return s3_loc.get + s3_loc = s3_client.get_bucket_location(Bucket=s3_bucket) + return s3_loc.get("LocationConstraint", "us-east-1") def _run_main(self, parsed_args, parsed_globals): region = parsed_globals.region if parsed_globals.region else "us-east-1" From 34b34a05737ecb9e4cbf879a2ec45c60bb4b7a47 Mon Sep 17 00:00:00 2001 From: Lessa Date: Thu, 11 Jan 2018 14:55:36 +0000 Subject: [PATCH 15/24] Removes unnecessary brackets --- awscli/customizations/cloudformation/package.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/awscli/customizations/cloudformation/package.py b/awscli/customizations/cloudformation/package.py index a0477d6658c7..606069504510 100644 --- a/awscli/customizations/cloudformation/package.py +++ b/awscli/customizations/cloudformation/package.py @@ -132,7 +132,7 @@ def _run_main(self, parsed_args, parsed_globals): raise exceptions.InvalidTemplatePathError( template_path=template_path) - if (parsed_args.s3_bucket is not None): + if parsed_args.s3_bucket is not None: bucket = parsed_args.s3_bucket s3_bucket_region = self._get_bucket_region(bucket, s3_client) if not s3_bucket_region == region: From 63e5b20c7401f056dd283a03badc07b4071527e6 Mon Sep 17 00:00:00 2001 From: Lessa Date: Thu, 11 Jan 2018 15:51:45 +0000 Subject: [PATCH 16/24] Adds EmptyRegion exception, refactor region var, fix LocationConstraint logic --- .../cloudformation/exceptions.py | 4 +++ .../customizations/cloudformation/package.py | 30 ++++++++++--------- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/awscli/customizations/cloudformation/exceptions.py b/awscli/customizations/cloudformation/exceptions.py index 7455ac950965..5901085d6a95 100644 --- a/awscli/customizations/cloudformation/exceptions.py +++ b/awscli/customizations/cloudformation/exceptions.py @@ -48,6 +48,10 @@ class DeployFailedError(CloudFormationCommandError): "aws cloudformation describe-stack-events --stack-name {stack_name}") +class PackageEmptyRegionError(CloudFormationCommandError): + fmt = "Failed to create package as AWS Region has not been defined" + + class PackageFailedRegionMismatchError(CloudFormationCommandError): fmt = \ ("Failed to create package as deployment and S3 bucket region mismatch" diff --git a/awscli/customizations/cloudformation/package.py b/awscli/customizations/cloudformation/package.py index 606069504510..bca84674c7ec 100644 --- a/awscli/customizations/cloudformation/package.py +++ b/awscli/customizations/cloudformation/package.py @@ -120,11 +120,13 @@ def _get_bucket_region(self, s3_bucket, s3_client): return s3_loc.get("LocationConstraint", "us-east-1") def _run_main(self, parsed_args, parsed_globals): - region = parsed_globals.region if parsed_globals.region else "us-east-1" + if not parsed_globals.region: + raise exceptions.PackageEmptyRegionError() + s3_client = self._session.create_client( "s3", config=Config(signature_version='s3v4'), - region_name=region, + region_name=parsed_globals.region, verify=parsed_globals.verify_ssl) template_path = parsed_args.template_file @@ -135,10 +137,10 @@ def _run_main(self, parsed_args, parsed_globals): if parsed_args.s3_bucket is not None: bucket = parsed_args.s3_bucket s3_bucket_region = self._get_bucket_region(bucket, s3_client) - if not s3_bucket_region == region: + if not s3_bucket_region == parsed_globals.region: raise exceptions.PackageFailedRegionMismatchError( bucket_region=s3_bucket_region, - deploy_region=region + deploy_region=parsed_globals.region ) else: sts_client = self._session.create_client( @@ -149,7 +151,7 @@ def _run_main(self, parsed_args, parsed_globals): account = sts_client.get_caller_identity().get('Account', "") bucket = "sam-{region}-{account}".format( account=account, - region=region + region=parsed_globals.region ) # Check if SAM deployment bucket already exists otherwise create it @@ -159,25 +161,25 @@ def _run_main(self, parsed_args, parsed_globals): if e.response["Error"]["Code"] == "404": sys.stdout.write( self.MSG_PACKAGE_S3_BUCKET_CREATION.format( - bucket=bucket, region=region)) + bucket=bucket, region=parsed_globals.region)) _s3_params = { "all_regions": { - "Bucket": bucket - }, - "us_standard": { "Bucket": bucket, "CreateBucketConfiguration": { - "LocationConstraint": region + "LocationConstraint": parsed_globals.region } + }, + "us_standard": { + "Bucket": bucket, } } - # Create bucket in specified region or else use us-east-1 - if parsed_globals.region: - s3_client.create_bucket(**_s3_params['all_regions']) - else: + # US Standard doesn't require CreateBucketConfiguration + if parsed_globals.region == "us-east-1": s3_client.create_bucket(**_s3_params['us_standard']) + else: + s3_client.create_bucket(**_s3_params['all_regions']) self.s3_uploader = S3Uploader(s3_client, bucket, From 8403b76eaf7b78c113577c5705788d617b63f030 Mon Sep 17 00:00:00 2001 From: Lessa Date: Thu, 11 Jan 2018 17:00:43 +0000 Subject: [PATCH 17/24] Covers Empty Region exception recently introduced --- .../cloudformation/test_package.py | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/tests/unit/customizations/cloudformation/test_package.py b/tests/unit/customizations/cloudformation/test_package.py index d7154a7ad9ad..2ea80ade4ea1 100644 --- a/tests/unit/customizations/cloudformation/test_package.py +++ b/tests/unit/customizations/cloudformation/test_package.py @@ -21,7 +21,7 @@ from awscli.customizations.cloudformation.package import PackageCommand from awscli.customizations.cloudformation.artifact_exporter import Template from awscli.customizations.cloudformation.yamlhelper import yaml_dump -from awscli.customizations.cloudformation.exceptions import PackageFailedRegionMismatchError +from awscli.customizations.cloudformation.exceptions import PackageFailedRegionMismatchError, PackageEmptyRegionError class FakeArgs(object): @@ -142,8 +142,19 @@ def test_main_bucket_different_deployment_region(self, mock_yaml_dump): self.package_command._run_main( self.parsed_args, self.parsed_globals) - self.package_command._export.reset_mock() - self.package_command.write_output.reset_mock() + def test_main_empty_region_error(self): + # Create a temporary file and make this my template + with tempfile.NamedTemporaryFile() as handle: + for use_json in (False, True): + filename = handle.name + self.parsed_args.template_file = filename + self.parsed_args.use_json = use_json + # None is what package receives if no --region is defined/passed + self.parsed_globals.region = None + + with self.assertRaises(PackageEmptyRegionError): + self.package_command._run_main( + self.parsed_args, self.parsed_globals) def test_main_error(self): From 277e7a37275dc6a9efcb7ad5adca2a37e37e7ea8 Mon Sep 17 00:00:00 2001 From: Lessa Date: Thu, 18 Jan 2018 22:26:40 +0000 Subject: [PATCH 18/24] Adds test to cover boto exception s3 bucket not found --- .../customizations/cloudformation/test_package.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/unit/customizations/cloudformation/test_package.py b/tests/unit/customizations/cloudformation/test_package.py index 2ea80ade4ea1..e8e54ff8c0ba 100644 --- a/tests/unit/customizations/cloudformation/test_package.py +++ b/tests/unit/customizations/cloudformation/test_package.py @@ -22,6 +22,7 @@ from awscli.customizations.cloudformation.artifact_exporter import Template from awscli.customizations.cloudformation.yamlhelper import yaml_dump from awscli.customizations.cloudformation.exceptions import PackageFailedRegionMismatchError, PackageEmptyRegionError +from botocore.exceptions import ClientError class FakeArgs(object): @@ -156,6 +157,18 @@ def test_main_empty_region_error(self): self.package_command._run_main( self.parsed_args, self.parsed_globals) + def test_bucket_already_exist_catch(self): + sts_client = Mock() + s3_client = Mock() + err_response = {'Error': {'Code': '404', 'Message': 'Not Found'}} + s3_client.head_bucket.side_effect = ClientError( + error_response=err_response, operation_name='HeadBucket') + + rc = self.package_command._create_sam_bucket( + s3_client, sts_client, self.parsed_globals) + # Test whether S3 Bucket Name has a 'sam-' prefix + self.assertIn("sam-", rc) + def test_main_error(self): self.package_command._export = Mock() From 00fe5c805c7ccfa9fc0886b2282ff031c7151e7a Mon Sep 17 00:00:00 2001 From: Lessa Date: Thu, 18 Jan 2018 22:29:06 +0000 Subject: [PATCH 19/24] Includes _get_bucket_region coverage without impacting test result --- tests/unit/customizations/cloudformation/test_package.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/unit/customizations/cloudformation/test_package.py b/tests/unit/customizations/cloudformation/test_package.py index e8e54ff8c0ba..c743a8416330 100644 --- a/tests/unit/customizations/cloudformation/test_package.py +++ b/tests/unit/customizations/cloudformation/test_package.py @@ -134,8 +134,6 @@ def test_main_bucket_different_deployment_region(self, mock_yaml_dump): self.parsed_args.template_file = filename self.parsed_args.use_json = use_json - self.package_command._get_bucket_region = MagicMock( - return_value="us-east-1") self.parsed_args.s3_bucket = "bucket-in-different-region" self.parsed_globals.region = "eu-west-1" From aab4cb145030b9f5f864c2fc18a02ed805ff33ea Mon Sep 17 00:00:00 2001 From: Lessa Date: Thu, 18 Jan 2018 22:30:51 +0000 Subject: [PATCH 20/24] Ran pep8 on test_package --- .../cloudformation/test_package.py | 45 ++++++++++--------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/tests/unit/customizations/cloudformation/test_package.py b/tests/unit/customizations/cloudformation/test_package.py index c743a8416330..8b69d4c6d1e7 100644 --- a/tests/unit/customizations/cloudformation/test_package.py +++ b/tests/unit/customizations/cloudformation/test_package.py @@ -60,7 +60,6 @@ def setUp(self): verify_ssl=None) self.package_command = PackageCommand(self.session) - @patch("awscli.customizations.cloudformation.package.yaml_dump") def test_main(self, mock_yaml_dump): exported_template_str = "hello" @@ -74,16 +73,19 @@ def test_main(self, mock_yaml_dump): for use_json in (False, True): filename = handle.name self.parsed_args.template_file = filename - self.parsed_args.use_json=use_json + self.parsed_args.use_json = use_json - self.package_command._get_bucket_region = MagicMock(return_value="us-east-1") + self.package_command._get_bucket_region = MagicMock( + return_value="us-east-1") - rc = self.package_command._run_main(self.parsed_args, self.parsed_globals) + rc = self.package_command._run_main( + self.parsed_args, self.parsed_globals) self.assertEquals(rc, 0) - self.package_command._export.assert_called_once_with(filename, use_json) + self.package_command._export.assert_called_once_with( + filename, use_json) self.package_command.write_output.assert_called_once_with( - self.parsed_args.output_template_file, mock.ANY) + self.parsed_args.output_template_file, mock.ANY) self.package_command._export.reset_mock() self.package_command.write_output.reset_mock() @@ -103,6 +105,7 @@ def test_main_without_bucket(self, mock_yaml_dump): self.parsed_args.template_file = filename self.parsed_args.use_json = use_json + # This can be deleted as it's not needed... self.package_command._get_bucket_region = MagicMock( return_value="us-east-1") self.parsed_args.s3_bucket = None @@ -149,24 +152,12 @@ def test_main_empty_region_error(self): self.parsed_args.template_file = filename self.parsed_args.use_json = use_json # None is what package receives if no --region is defined/passed - self.parsed_globals.region = None + self.parsed_globals.region = None with self.assertRaises(PackageEmptyRegionError): self.package_command._run_main( self.parsed_args, self.parsed_globals) - def test_bucket_already_exist_catch(self): - sts_client = Mock() - s3_client = Mock() - err_response = {'Error': {'Code': '404', 'Message': 'Not Found'}} - s3_client.head_bucket.side_effect = ClientError( - error_response=err_response, operation_name='HeadBucket') - - rc = self.package_command._create_sam_bucket( - s3_client, sts_client, self.parsed_globals) - # Test whether S3 Bucket Name has a 'sam-' prefix - self.assertIn("sam-", rc) - def test_main_error(self): self.package_command._export = Mock() @@ -178,11 +169,23 @@ def test_main_error(self): self.parsed_args.template_file = filename self.package_command._get_bucket_region = MagicMock( - return_value="us-east-1") + return_value="us-east-1") with self.assertRaises(RuntimeError): - self.package_command._run_main(self.parsed_args, self.parsed_globals) + self.package_command._run_main( + self.parsed_args, self.parsed_globals) + def test_bucket_already_exist_catch(self): + sts_client = Mock() + s3_client = Mock() + err_response = {'Error': {'Code': '404', 'Message': 'Not Found'}} + s3_client.head_bucket.side_effect = ClientError( + error_response=err_response, operation_name='HeadBucket') + + rc = self.package_command._create_sam_bucket( + s3_client, sts_client, self.parsed_globals) + # Test whether S3 Bucket Name has a 'sam-' prefix + self.assertIn("sam-", rc) @patch("awscli.customizations.cloudformation.package.sys.stdout") def test_write_output_to_stdout(self, stdoutmock): From e96d568483d1686d5ef0511b376ebf4ab6e7bf9d Mon Sep 17 00:00:00 2001 From: Lessa Date: Thu, 18 Jan 2018 22:31:50 +0000 Subject: [PATCH 21/24] refactor to increase code coverage in unit test --- .../customizations/cloudformation/package.py | 103 ++++++++++++------ 1 file changed, 71 insertions(+), 32 deletions(-) diff --git a/awscli/customizations/cloudformation/package.py b/awscli/customizations/cloudformation/package.py index bca84674c7ec..6f35cf46dc9f 100644 --- a/awscli/customizations/cloudformation/package.py +++ b/awscli/customizations/cloudformation/package.py @@ -119,6 +119,42 @@ def _get_bucket_region(self, s3_bucket, s3_client): s3_loc = s3_client.get_bucket_location(Bucket=s3_bucket) return s3_loc.get("LocationConstraint", "us-east-1") + def _create_sam_bucket(self, s3_client, sts_client, parsed_globals): + """ Creates random S3 Bucket for SAM Packaging """ + account = sts_client.get_caller_identity().get('Account', "") + bucket = "sam-{region}-{account}".format( + account=account, + region=parsed_globals.region + ) + + # Check if SAM deployment bucket already exists otherwise create it + try: + s3_client.head_bucket(Bucket=bucket) + except ClientError as e: + if e.response["Error"]["Code"] == "404": + sys.stdout.write( + self.MSG_PACKAGE_S3_BUCKET_CREATION.format( + bucket=bucket, region=parsed_globals.region)) + + s3_params = { + "all_regions": { + "Bucket": bucket, + "CreateBucketConfiguration": { + "LocationConstraint": parsed_globals.region + } + }, + "us_standard": { + "Bucket": bucket, + } + } + + if parsed_globals.region == "us-east-1": + s3_client.create_bucket(**s3_params['us_standard']) + else: + s3_client.create_bucket(**s3_params['all_regions']) + + return bucket + def _run_main(self, parsed_args, parsed_globals): if not parsed_globals.region: raise exceptions.PackageEmptyRegionError() @@ -148,38 +184,41 @@ def _run_main(self, parsed_args, parsed_globals): config=Config(signature_version='s3v4'), verify=parsed_globals.verify_ssl ) - account = sts_client.get_caller_identity().get('Account', "") - bucket = "sam-{region}-{account}".format( - account=account, - region=parsed_globals.region - ) - - # Check if SAM deployment bucket already exists otherwise create it - try: - s3_client.head_bucket(Bucket=bucket) - except ClientError as e: - if e.response["Error"]["Code"] == "404": - sys.stdout.write( - self.MSG_PACKAGE_S3_BUCKET_CREATION.format( - bucket=bucket, region=parsed_globals.region)) - - _s3_params = { - "all_regions": { - "Bucket": bucket, - "CreateBucketConfiguration": { - "LocationConstraint": parsed_globals.region - } - }, - "us_standard": { - "Bucket": bucket, - } - } - - # US Standard doesn't require CreateBucketConfiguration - if parsed_globals.region == "us-east-1": - s3_client.create_bucket(**_s3_params['us_standard']) - else: - s3_client.create_bucket(**_s3_params['all_regions']) + bucket = self._create_sam_bucket( + s3_client, sts_client, parsed_globals) + + # account = sts_client.get_caller_identity().get('Account', "") + # bucket = "sam-{region}-{account}".format( + # account=account, + # region=parsed_globals.region + # ) + + # # Check if SAM deployment bucket already exists otherwise create it + # try: + # s3_client.head_bucket(Bucket=bucket) + # except ClientError as e: + # if e.response["Error"]["Code"] == "404": + # sys.stdout.write( + # self.MSG_PACKAGE_S3_BUCKET_CREATION.format( + # bucket=bucket, region=parsed_globals.region)) + + # _s3_params = { + # "all_regions": { + # "Bucket": bucket, + # "CreateBucketConfiguration": { + # "LocationConstraint": parsed_globals.region + # } + # }, + # "us_standard": { + # "Bucket": bucket, + # } + # } + + # # US Standard doesn't require CreateBucketConfiguration + # if parsed_globals.region == "us-east-1": + # s3_client.create_bucket(**_s3_params['us_standard']) + # else: + # s3_client.create_bucket(**_s3_params['all_regions']) self.s3_uploader = S3Uploader(s3_client, bucket, From fcb21ca46db197b7e54fe034b9ad591326c7b941 Mon Sep 17 00:00:00 2001 From: Lessa Date: Thu, 18 Jan 2018 22:31:50 +0000 Subject: [PATCH 22/24] refactor to increase code coverage in unit test --- .../customizations/cloudformation/package.py | 70 ++++++++++--------- .../cloudformation/test_package.py | 12 +++- 2 files changed, 47 insertions(+), 35 deletions(-) diff --git a/awscli/customizations/cloudformation/package.py b/awscli/customizations/cloudformation/package.py index bca84674c7ec..3e9814f2b556 100644 --- a/awscli/customizations/cloudformation/package.py +++ b/awscli/customizations/cloudformation/package.py @@ -119,6 +119,42 @@ def _get_bucket_region(self, s3_bucket, s3_client): s3_loc = s3_client.get_bucket_location(Bucket=s3_bucket) return s3_loc.get("LocationConstraint", "us-east-1") + def _create_sam_bucket(self, s3_client, sts_client, parsed_globals): + """ Creates random S3 Bucket for SAM Packaging """ + account = sts_client.get_caller_identity().get('Account', "") + bucket = "sam-{region}-{account}".format( + account=account, + region=parsed_globals.region + ) + + # Check if SAM deployment bucket already exists otherwise create it + try: + s3_client.head_bucket(Bucket=bucket) + except ClientError as e: + if e.response["Error"]["Code"] == "404": + sys.stdout.write( + self.MSG_PACKAGE_S3_BUCKET_CREATION.format( + bucket=bucket, region=parsed_globals.region)) + + s3_params = { + "all_regions": { + "Bucket": bucket, + "CreateBucketConfiguration": { + "LocationConstraint": parsed_globals.region + } + }, + "us_standard": { + "Bucket": bucket, + } + } + + if parsed_globals.region == "us-east-1": + s3_client.create_bucket(**s3_params['us_standard']) + else: + s3_client.create_bucket(**s3_params['all_regions']) + + return bucket + def _run_main(self, parsed_args, parsed_globals): if not parsed_globals.region: raise exceptions.PackageEmptyRegionError() @@ -148,38 +184,8 @@ def _run_main(self, parsed_args, parsed_globals): config=Config(signature_version='s3v4'), verify=parsed_globals.verify_ssl ) - account = sts_client.get_caller_identity().get('Account', "") - bucket = "sam-{region}-{account}".format( - account=account, - region=parsed_globals.region - ) - - # Check if SAM deployment bucket already exists otherwise create it - try: - s3_client.head_bucket(Bucket=bucket) - except ClientError as e: - if e.response["Error"]["Code"] == "404": - sys.stdout.write( - self.MSG_PACKAGE_S3_BUCKET_CREATION.format( - bucket=bucket, region=parsed_globals.region)) - - _s3_params = { - "all_regions": { - "Bucket": bucket, - "CreateBucketConfiguration": { - "LocationConstraint": parsed_globals.region - } - }, - "us_standard": { - "Bucket": bucket, - } - } - - # US Standard doesn't require CreateBucketConfiguration - if parsed_globals.region == "us-east-1": - s3_client.create_bucket(**_s3_params['us_standard']) - else: - s3_client.create_bucket(**_s3_params['all_regions']) + bucket = self._create_sam_bucket( + s3_client, sts_client, parsed_globals) self.s3_uploader = S3Uploader(s3_client, bucket, diff --git a/tests/unit/customizations/cloudformation/test_package.py b/tests/unit/customizations/cloudformation/test_package.py index 8b69d4c6d1e7..8b2fe6e39c9f 100644 --- a/tests/unit/customizations/cloudformation/test_package.py +++ b/tests/unit/customizations/cloudformation/test_package.py @@ -182,10 +182,16 @@ def test_bucket_already_exist_catch(self): s3_client.head_bucket.side_effect = ClientError( error_response=err_response, operation_name='HeadBucket') - rc = self.package_command._create_sam_bucket( + rc_default_region = self.package_command._create_sam_bucket( s3_client, sts_client, self.parsed_globals) - # Test whether S3 Bucket Name has a 'sam-' prefix - self.assertIn("sam-", rc) + + self.parsed_globals.region = "eu-west-1" + rc_custom_region = self.package_command._create_sam_bucket( + s3_client, sts_client, self.parsed_globals) + + self.assertIn("sam-", rc_default_region) + self.assertIn("sam-", rc_custom_region) + @patch("awscli.customizations.cloudformation.package.sys.stdout") def test_write_output_to_stdout(self, stdoutmock): From 3661e4229a600d292149701969faacc59bdbd286 Mon Sep 17 00:00:00 2001 From: Lessa Date: Sat, 20 Jan 2018 20:52:05 +0000 Subject: [PATCH 23/24] Clean up, refactor even original tests to reach 100% coverage --- .../cloudformation/test_package.py | 139 ++++++++---------- 1 file changed, 60 insertions(+), 79 deletions(-) diff --git a/tests/unit/customizations/cloudformation/test_package.py b/tests/unit/customizations/cloudformation/test_package.py index 8b2fe6e39c9f..6b728c662edc 100644 --- a/tests/unit/customizations/cloudformation/test_package.py +++ b/tests/unit/customizations/cloudformation/test_package.py @@ -18,10 +18,12 @@ from io import StringIO from mock import patch, Mock, MagicMock from awscli.testutils import unittest, BaseAWSCommandParamsTest -from awscli.customizations.cloudformation.package import PackageCommand -from awscli.customizations.cloudformation.artifact_exporter import Template +from awscli.customizations.cloudformation.package import ( + PackageCommand, Template, json) from awscli.customizations.cloudformation.yamlhelper import yaml_dump -from awscli.customizations.cloudformation.exceptions import PackageFailedRegionMismatchError, PackageEmptyRegionError +from awscli.customizations.cloudformation.exceptions import ( + PackageFailedRegionMismatchError, PackageEmptyRegionError, + InvalidTemplatePathError) from botocore.exceptions import ClientError @@ -60,103 +62,75 @@ def setUp(self): verify_ssl=None) self.package_command = PackageCommand(self.session) + @patch("awscli.customizations.cloudformation.package.json") + @patch("awscli.customizations.cloudformation.package.Template") @patch("awscli.customizations.cloudformation.package.yaml_dump") - def test_main(self, mock_yaml_dump): + def test_main(self, mock_yaml_dump, mock_export_template, mock_json_dump): exported_template_str = "hello" - - self.package_command.write_output = Mock() - self.package_command._export = Mock() mock_yaml_dump.return_value = exported_template_str + mock_json_dump.dumps.return_value = "template_json_format" # Create a temporary file and make this my template with tempfile.NamedTemporaryFile() as handle: - for use_json in (False, True): - filename = handle.name - self.parsed_args.template_file = filename - self.parsed_args.use_json = use_json - - self.package_command._get_bucket_region = MagicMock( - return_value="us-east-1") - - rc = self.package_command._run_main( - self.parsed_args, self.parsed_globals) - self.assertEquals(rc, 0) - - self.package_command._export.assert_called_once_with( - filename, use_json) - self.package_command.write_output.assert_called_once_with( - self.parsed_args.output_template_file, mock.ANY) - - self.package_command._export.reset_mock() - self.package_command.write_output.reset_mock() - - @patch("awscli.customizations.cloudformation.package.yaml_dump") - def test_main_without_bucket(self, mock_yaml_dump): - exported_template_str = "hello" - - self.package_command.write_output = Mock() - self.package_command._export = Mock() - mock_yaml_dump.return_value = exported_template_str + self.package_command._get_bucket_region = MagicMock( + return_value="us-east-1") - # Create a temporary file and make this my template - with tempfile.NamedTemporaryFile() as handle: for use_json in (False, True): filename = handle.name self.parsed_args.template_file = filename self.parsed_args.use_json = use_json - # This can be deleted as it's not needed... - self.package_command._get_bucket_region = MagicMock( - return_value="us-east-1") - self.parsed_args.s3_bucket = None - rc = self.package_command._run_main( self.parsed_args, self.parsed_globals) self.assertEquals(rc, 0) - self.package_command._export.assert_called_once_with( - filename, use_json) - self.package_command.write_output.assert_called_once_with( - self.parsed_args.output_template_file, mock.ANY) - - self.package_command._export.reset_mock() - self.package_command.write_output.reset_mock() + self.package_command.write_output( + self.parsed_args.output_template_file, "some data") - @patch("awscli.customizations.cloudformation.package.yaml_dump") - def test_main_bucket_different_deployment_region(self, mock_yaml_dump): - exported_template_str = "hello" + # No output file specified + self.parsed_args.output_template_file = None + rc_no_output = self.package_command._run_main( + self.parsed_args, self.parsed_globals) + self.assertEquals(rc_no_output, 0) + @patch("os.path.isfile") + def test_main_without_bucket(self, mock_os_isfile): self.package_command.write_output = Mock() self.package_command._export = Mock() - mock_yaml_dump.return_value = exported_template_str + mock_os_isfile.return_value = True - # Create a temporary file and make this my template - with tempfile.NamedTemporaryFile() as handle: - for use_json in (False, True): - filename = handle.name - self.parsed_args.template_file = filename - self.parsed_args.use_json = use_json + self.parsed_args.s3_bucket = None + self.package_command._get_bucket_region = MagicMock( + return_value="us-east-1") + rc = self.package_command._run_main( + self.parsed_args, self.parsed_globals) - self.parsed_args.s3_bucket = "bucket-in-different-region" - self.parsed_globals.region = "eu-west-1" + self.assertEquals(rc, 0) - with self.assertRaises(PackageFailedRegionMismatchError): - self.package_command._run_main( - self.parsed_args, self.parsed_globals) + @patch("os.path.isfile") + def test_main_bucket_region_mismatch(self, mock_os_isfile): + mock_os_isfile.return_value = True + self.parsed_globals.region = "eu-west-1" + self.parsed_args.s3_bucket = "bucket-in-different-region" - def test_main_empty_region_error(self): - # Create a temporary file and make this my template - with tempfile.NamedTemporaryFile() as handle: - for use_json in (False, True): - filename = handle.name - self.parsed_args.template_file = filename - self.parsed_args.use_json = use_json - # None is what package receives if no --region is defined/passed - self.parsed_globals.region = None + with self.assertRaises(PackageFailedRegionMismatchError): + self.package_command._run_main( + self.parsed_args, self.parsed_globals + ) - with self.assertRaises(PackageEmptyRegionError): - self.package_command._run_main( - self.parsed_args, self.parsed_globals) + def test_main_empty_region_error(self): + self.parsed_globals.region = None + with self.assertRaises(PackageEmptyRegionError): + self.package_command._run_main( + self.parsed_args, self.parsed_globals + ) + + def test_main_invalid_template_error(self): + self.parsed_args.template_file = "invalid-template-file" + with self.assertRaises(InvalidTemplatePathError): + self.package_command._run_main( + self.parsed_args, self.parsed_globals + ) def test_main_error(self): @@ -185,18 +159,25 @@ def test_bucket_already_exist_catch(self): rc_default_region = self.package_command._create_sam_bucket( s3_client, sts_client, self.parsed_globals) + self.assertIn("sam-", rc_default_region) + self.parsed_globals.region = "eu-west-1" rc_custom_region = self.package_command._create_sam_bucket( s3_client, sts_client, self.parsed_globals) - self.assertIn("sam-", rc_default_region) self.assertIn("sam-", rc_custom_region) + # Covers non-404 use case + err_response['Error']['Code'] = "418" + err_response['Error']['Message'] = "Easter egg, return bucket" + rc_custom_region = self.package_command._create_sam_bucket( + s3_client, sts_client, self.parsed_globals) + + self.assertIn("sam-", rc_custom_region) @patch("awscli.customizations.cloudformation.package.sys.stdout") def test_write_output_to_stdout(self, stdoutmock): data = u"some data" - filename = None - self.package_command.write_output(filename, data) - stdoutmock.write.assert_called_once_with(data) + for filename in (self.parsed_args.output_template_file, None): + rc = self.package_command.write_output(filename, data) From d55263476f744b8724511d6a9e804ec0a6e77074 Mon Sep 17 00:00:00 2001 From: Lessa Date: Sat, 20 Jan 2018 22:27:52 +0000 Subject: [PATCH 24/24] typo --- tests/unit/customizations/cloudformation/test_package.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/customizations/cloudformation/test_package.py b/tests/unit/customizations/cloudformation/test_package.py index 6b728c662edc..2b8bb59cd932 100644 --- a/tests/unit/customizations/cloudformation/test_package.py +++ b/tests/unit/customizations/cloudformation/test_package.py @@ -55,7 +55,7 @@ def setUp(self): s3_bucket="s3bucket", s3_prefix="s3prefix", kms_key_id="kmskeyid", - output_template_file="./oputput", + output_template_file="./output", use_json=False, force_upload=False) self.parsed_globals = FakeArgs(region="us-east-1", endpoint_url=None,