S3 commands ensure error RC takes precedence #2998

joguSD · 2017-11-29T01:16:03Z

If an S3 command encounters an error and a warning it will use the warning RC masking that the error happened. This adds a test for this case and ensures we use the correct RC.

kyleknap

Looks good to me. Just had a comment about moving the test to a functional test instead of an integration test.

kyleknap · 2017-12-04T23:21:39Z

tests/integration/customizations/s3/test_plugin.py

@@ -1055,6 +1055,33 @@ def test_bad_symlink(self):
                      p.stderr)


+class TestErrorAndWarning(BaseS3IntegrationTest):


What is your thoughts on moving this to a functional test? It would be better because we run the functional tests more frequently and we do not necessarily need to send the request over the wire to test for this.

I originally looked into implementing this as a functional test but opted for an integration test because I wasn't sure if I could guarantee the order that the requests are made. All examples of the functional tests for these currently only have a single line of api request logic and not two. Basically, will the S3 transfer manager guarantee the order of the put_object calls?

So you could set the max_concurrent_requests rate to 1 to avoid that and it is fine that we would be overriding the config file because we would not be making an API requests. From there you could recreate a scenario that would trigger this. There are a probably a handful that you can do but one would be to do a recursive upload and make one of the files a special file type so it gets skipped and the other be normal and have the PutObject fail. This way you do not have to override the max_concurrent_requests either. The one downside is that we would have to skip for Windows with this approach specifically for the special file. But if you can think of something that does not skip Windows that would be ideal, maybe look around in the tests for cases where we skip?

Actually instead of a special file, you could use the set_invalid_utime from before and that should allows us to avoid skipping on Windows.

Looks like this is just pending this feedback getting incorporated right? Where did we land on this?

Yep, gonna update this to become a functional test.

codecov-io · 2017-12-06T22:13:35Z

Codecov Report

Merging #2998 into develop will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           develop    #2998   +/-   ##
========================================
  Coverage    95.76%   95.76%           
========================================
  Files          165      165           
  Lines        12575    12575           
========================================
  Hits         12042    12042           
  Misses         533      533

Impacted Files	Coverage Δ
awscli/customizations/s3/subcommands.py	`97.16% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9ebd9e...3ea7eab. Read the comment docs.

kyleknap

Nice! 🚢

* sam pipeline bootstrap (aws#2811) * two-stages-pipeline plugin * typos * add docstring * make mypy happy * removing swap file * delete the two_stages_pipeline plugin as the pipeline-bootstrap command took over its responsibility * remove 'get_template_function_runtimes' function as the decision is made to not process the SAM template during pipeline init which was the only place we use the function * sam pipeline bootstrap command * move the pipelineconfig.toml file to .aws-sam * UX - rewriting Co-authored-by: Chris Rehn <crehn@outlook.com> * UX improvements * make black happy * apply review comments * UX - rewriting Co-authored-by: Chris Rehn <crehn@outlook.com> * refactor * Apply review comments * use python way of array elements assignments * Update samcli/lib/pipeline/bootstrap/stage.py Co-authored-by: _sam <3804518+aahung@users.noreply.github.com> * apply review comments * typo * read using utf-8 * create and user a safe version of the save_config method * apply review comments * rename _get_command_name to _get_command_names * don't save generated ARNs for now, will save during init * Revert "don't save generated ARNs for now, will save during init" This reverts commit d184e164022d9560131c62a826436edbc93da189. * Notify the user to rotate periodically rotate the IAM credentials * typo * Use AES instead of KMS for S3 SSE * rename Ecr to ECR and Iam to IAM * Grant lambda service explicit permissions to thhe ECR instead of relying on giving this permissions on ad-hoc while creating the container images Co-authored-by: Chris Rehn <crehn@outlook.com> Co-authored-by: _sam <3804518+aahung@users.noreply.github.com> * sam pipeline init command (aws#2831) * sam pipeline init command * apply review comments * apply review comments * display a message that we have successfully created the pipeline configuration file(s). * doc typo * Let 'sam pipeline init' prefills pipeline's infrastructure resources… (aws#2894) * Let 'sam pipeline init' prefills pipeline's infrastructure resources' values from 'sam pipeline bootstrap' results. * save bootstrapped sateg region * make black happy * exclude non-dict keys from samconfig.get_env_names method. * Rename the pipeline 'Stage' concept to 'Environment' (aws#2908) * Rename the pipeline 'Stage' concept to 'Environment' * typo * Rename --environment-name argument to --environment * Sam pipelines ux rename ecr repo to image repository (aws#2910) * Rename ecr-repo to image-repository * UT Fixes * typo * typo * feat: Support creating pipeline files directly into . without hooks (aws#2911) * feat: Support creating pipeline files directly into . without hooks * Integration test for pipeline init and pipeline bootstrap (aws#2841) * Expose Environment._get_stack_name for integ test to predict stack name * Add integ test for pipeline bootstrap * Add init integ test * small UX improvements: (aws#2914) * small UX improvements: 1. show a message when the user cancels a bootstrapping command. 2. Don't prompt for CI/CD provider or provider templates if there is only one choice. 3. Make PipelineFileAlreadyExistsError a UserError. 4. use the Colored class instead of fg='color' when prompting a colored message. 5. Fix a bug where we were not allowing empty response for not required questions. * Fix Integration Test: We now don't ask the user to select a provider's pipeline template if there is only one * Add docs for PipelineFileAlreadyExistsError * make black happy * Sam pipelines s3 security (aws#2975) * Deny non https requests for the artifacts S3 bucket * enable bucket serverside logging * add integration tests for artifacts bucket SSL-only requests and access logging * typo * Ensure the ArtifactsLoggingBucket denies non ssl requests (aws#2976) * Sam pipelines ux round 3 (aws#2979) * rename customer facing message 'CI/CD provider' to 'CI/CD system' * add a note about what 'Environment Name' is during the pipeline bootstrap guided context * Apply suggestions from code review typo Co-authored-by: Chris Rehn <crehn@outlook.com> Co-authored-by: Chris Rehn <crehn@outlook.com> * let pipeline IAM user assume only IAM roles tagged with Role=pipeline-execution-role (aws#2982) * Adding AWS_ prefix to displayed out. (aws#2993) Co-authored-by: Tarun Mall <tarun@amazon.noreply.github.com> * Add region to pipeline bootstrap interactive flow (aws#2997) * Ask AWS region in bootstrap interactive flow * Read default region from boto session first * Fix a unit test * Inform write to pipelineconfig.toml at the end of bootstrap (aws#3002) * Print info about pipelineconfig.toml after resources are bootstrapped * Update samcli/commands/pipeline/bootstrap/cli.py Co-authored-by: Chris Rehn <crehn@outlook.com> Co-authored-by: Chris Rehn <crehn@outlook.com> * List detected env names in pipeline init when prompt to input the env name (aws#3000) * Allow question.question can be resolved using key path * Pass the list of env names message (environment_names_message) into pipeline init interactive flow context * Update samcli/commands/pipeline/init/interactive_init_flow.py Co-authored-by: Chris Rehn <crehn@outlook.com> * Fix unit test (trigger pr builds) * Fix integ test * Fix integ test Co-authored-by: Chris Rehn <crehn@outlook.com> * Adding account id to bootstrap message. (aws#2998) * Adding account id to bootstrap message. * adding docstring * Addressing PR comments. * Adding unit tests. * Fixing unit tests. Co-authored-by: Tarun Mall <tarun@amazon.noreply.github.com> * Cfn creds fix (aws#3014) * Removing pipeline user creds from cfn output. This maintains same user exp. Co-authored-by: Tarun Mall <tarun@amazon.noreply.github.com> * Ux bootstrap revamp 20210706 (aws#3021) * Add intro paragraph to bootstrap * Add switch account prompt * Revamp stage definition prompt * Revamp existing resources prompt * Revamp security prompt * Allow answers to be changed later * Add exit message for bootstrap * Add exit message for bootstrap (1) * Add indentation to review values * Add "Below is the summary of the answers:" * Sweep pylint errors * Update unit tests * Update samcli/commands/pipeline/bootstrap/guided_context.py Co-authored-by: Chris Rehn <crehn@outlook.com> * Update samcli/commands/pipeline/bootstrap/guided_context.py Co-authored-by: Chris Rehn <crehn@outlook.com> * Update samcli/commands/pipeline/bootstrap/guided_context.py Co-authored-by: Chris Rehn <crehn@outlook.com> * Update samcli/commands/pipeline/bootstrap/guided_context.py Co-authored-by: Chris Rehn <crehn@outlook.com> * Update samcli/commands/pipeline/bootstrap/guided_context.py Co-authored-by: Chris Rehn <crehn@outlook.com> * Update samcli/commands/pipeline/bootstrap/guided_context.py Co-authored-by: Chris Rehn <crehn@outlook.com> * Update samcli/commands/pipeline/bootstrap/guided_context.py Co-authored-by: Chris Rehn <crehn@outlook.com> * Update samcli/commands/pipeline/bootstrap/guided_context.py Co-authored-by: Chris Rehn <crehn@outlook.com> * Update samcli/commands/pipeline/bootstrap/cli.py Co-authored-by: Chris Rehn <crehn@outlook.com> * Update unit tests * Add bold to other literals Co-authored-by: Chris Rehn <crehn@outlook.com> * Adding account condition for CFN execution role. (aws#3027) Co-authored-by: Tarun Mall <tarun@amazon.noreply.github.com> * pipeline UX revamp 20210707 (aws#3031) * Allow running bootstrap inside pipeline init * Select account credential source within bootstrap * Add bootstrap decorations within pipeline init * Removing ip range option from bootstrap. (aws#3036) * Removing ip range option from bootstrap. * Fixing unit test from UX PR. Co-authored-by: Tarun Mall <tarun@amazon.noreply.github.com> * Fix toml file incorrect read/write in init --bootstrap (aws#3037) * Temporarily removing account fix. (aws#3038) Co-authored-by: Tarun Mall <tarun@amazon.noreply.github.com> * Rename environment to stage (aws#3040) * Improve account source selection (aws#3042) * Fixing various cosmetics UX issues with pipeline workflow. (aws#3046) * Fixing credential to credentials * Forcing text color to yellow. * Adding new line after stage diagram. * Adding extra line after checking bootstrap message. * Renaming config -> configuration * account source -> credential source * Removing old message. * Fixing indentation in list. * Fixing bunch of indentation. * fixing f string Co-authored-by: Tarun Mall <tarun@amazon.noreply.github.com> * Auto skip questions if stage detected (aws#3045) * Autofill question if default value is presented * Allow to use index to select stage names (aws#3051) * Updating message when bootstrap stages are missing. (aws#3058) * Updating message when bootstrap stages are missing. * Fixing indendation Co-authored-by: Tarun Mall <tarun@amazon.noreply.github.com> * Fixing bootstrap integ tests. (aws#3061) * Fixing bootstrap integ tests. * Cleaning up some integ tests. * Using environment variables when running integ test on CI. * Using expression instead of full loop. * Adding instruction to use default profile on local. Co-authored-by: Tarun Mall <tarun@amazon.noreply.github.com> * Fix bootstrap test region (#3064) * Fix bootstrap region in integ test * Fix regions in non-interactive mode as well * Add more pipeline init integ test (aws#3065) * Fix existing pipeline init integ test * Add more pipeline init integ tests * Config file bug (aws#3066) * Validating config file after bootstrap stack creation. * Validating config file after bootstrap. Co-authored-by: Tarun Mall <tarun@amazon.noreply.github.com> * Fix pipeline init integ test because of pipelineconfig file exists (aws#3067) * Make stage name randomized to avoid race condition among multi canary runs (aws#3078) * Load number of stages from pipeline template (aws#3059) * Load number of stages from templates * Rename variable and add debug log * Add encoding to open() * Allow roles with Tag aws-sam-pipeline-codebuild-service-role to assume PipelineExecutionRole (aws#2950) * pipeline init UX: Ask to confirm when file exists (aws#3079) * Ask to confirm overriding if files already exist, or save to another directory * Add doc links (aws#3087) * Adding accidentally removed tests back. (aws#3088) Co-authored-by: Tarun Mall <tarun@amazon.noreply.github.com> Co-authored-by: elbayaaa <72949274+elbayaaa@users.noreply.github.com> Co-authored-by: Chris Rehn <crehn@outlook.com> Co-authored-by: Ahmed Elbayaa <elbayaaa@amazon.com> Co-authored-by: Tarun <c2tarun@users.noreply.github.com> Co-authored-by: Tarun Mall <tarun@amazon.noreply.github.com>

joguSD requested review from dstufft, jamesls, stealthycoin, JordonPhillips and kyleknap November 29, 2017 01:16

kyleknap reviewed Dec 4, 2017

View reviewed changes

JordonPhillips approved these changes Dec 4, 2017

View reviewed changes

Ensure s3 subcommands return RC 1 on failure

1d0643d

joguSD force-pushed the s3-rc branch from b7b113f to c5265d5 Compare December 6, 2017 22:13

kyleknap approved these changes Dec 6, 2017

View reviewed changes

Add a test for s3 commands that have an error and warning

3ea7eab

joguSD force-pushed the s3-rc branch from c5265d5 to 3ea7eab Compare December 7, 2017 17:21

joguSD merged commit cc63276 into aws:develop Dec 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

S3 commands ensure error RC takes precedence #2998

S3 commands ensure error RC takes precedence #2998

joguSD commented Nov 29, 2017

kyleknap left a comment

kyleknap Dec 4, 2017

joguSD Dec 4, 2017

kyleknap Dec 4, 2017

kyleknap Dec 4, 2017

jamesls Dec 6, 2017

joguSD Dec 6, 2017

codecov-io commented Dec 6, 2017 •

edited

Loading

kyleknap left a comment

		@@ -1055,6 +1055,33 @@ def test_bad_symlink(self):
		p.stderr)


		class TestErrorAndWarning(BaseS3IntegrationTest):

S3 commands ensure error RC takes precedence #2998

S3 commands ensure error RC takes precedence #2998

Conversation

joguSD commented Nov 29, 2017

kyleknap left a comment

Choose a reason for hiding this comment

kyleknap Dec 4, 2017

Choose a reason for hiding this comment

joguSD Dec 4, 2017

Choose a reason for hiding this comment

kyleknap Dec 4, 2017

Choose a reason for hiding this comment

kyleknap Dec 4, 2017

Choose a reason for hiding this comment

jamesls Dec 6, 2017

Choose a reason for hiding this comment

joguSD Dec 6, 2017

Choose a reason for hiding this comment

codecov-io commented Dec 6, 2017 • edited Loading

Codecov Report

kyleknap left a comment

Choose a reason for hiding this comment

codecov-io commented Dec 6, 2017 •

edited

Loading