Skip to content

fix: modify session and kms_utils to check for S3 bucket before creation #1271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
Jan 29, 2020
Merged

fix: modify session and kms_utils to check for S3 bucket before creation #1271

merged 9 commits into from
Jan 29, 2020

Conversation

knakad
Copy link
Contributor

@knakad knakad commented Jan 29, 2020

Issue #, if available: #1258

Description of changes:
Previously, the code would create the bucket and enact business logic
based on the exceptions thrown.
This commit makes it such that the code checks that the bucket exists
before trying to create it.

In addition to being cleaner, this also avoids issues if customers
have no S3 bucket creation permissions.

Testing done:
UTs. PR will run ITs.

Merge Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.

General

  • I have read the CONTRIBUTING doc
  • I used the commit message format described in CONTRIBUTING
  • I have used the regional endpoint when creating S3 and/or STS clients (if appropriate)
  • I have updated any necessary documentation, including READMEs and API docs (if appropriate)

Tests

  • I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • I have checked that my tests are not configured for a specific region or account (if appropriate)
  • I have used unique_name_from_base to create resource names in integ tests (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@knakad
modify session and kms_utils to check for S3 bucket before creation
8dd4153 
Previously, the code would create the bucket and enact business logic
based on the exceptions thrown.
This commit makes it such that the code checks that the bucket exists
before trying to create it.

In addition to being cleaner, this also avoids issues if customers
have no S3 bucket creation permissions.
@knakad knakad requested a review from laurenyu January 29, 2020 01:07
laurenyu
laurenyu previously requested changes Jan 29, 2020
View reviewed changes
Copy link
Contributor

@laurenyu laurenyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking on needing region_name for S3 client/resource instantiation

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@knakad knakad changed the title modify session and kms_utils to check for S3 bucket before creation fix: modify session and kms_utils to check for S3 bucket before creation Jan 29, 2020
@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

laurenyu
laurenyu reviewed Jan 29, 2020
View reviewed changes
@laurenyu laurenyu dismissed their stale review January 29, 2020 18:15

region_name issue has been addressed, so no need for a hard block on merging.

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

laurenyu
laurenyu reviewed Jan 29, 2020
View reviewed changes
@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@knakad knakad merged commit ae3247a into aws:master Jan 29, 2020
@knakad knakad deleted the 20200128-check-bucket-before-create branch January 29, 2020 22:32
benieric pushed a commit that referenced this pull request Nov 29, 2023
@nmadan @rohangujarathi
@Zhankuil @svia3 @stacicho @edwardps @knikure
feature: Add Pipeline step decorator, NotebookJobStep, and scheduler (#…
20cd3b6 
…1271)

Co-authored-by: Rohan Gujarathi <[email protected]>
Co-authored-by: svia3 <[email protected]>
Co-authored-by: Zhankui Lu <[email protected]>
Co-authored-by: Dewen Qi <[email protected]>
Co-authored-by: Edward Sun <[email protected]>
Co-authored-by: Stephen Via <[email protected]>
Co-authored-by: Namrata Madan <[email protected]>
Co-authored-by: Stacia Choe <[email protected]>
Co-authored-by: Edward Sun <[email protected]>
Co-authored-by: Edward Sun <[email protected]>
Co-authored-by: Rohan Gujarathi <[email protected]>
fix: Multiple bug fixes including removing unsupported feature. (#1105)
Fix some problems with pipeline compilation (#1125)
fix: Refactor JsonGet s3 URI and add serialize_output_to_json flag (#1164)
fix: invoke_function circular import (#1262)
fix: pylint (#1264)
fix: Add logging for docker build failures (#1267)
benieric pushed a commit that referenced this pull request Nov 29, 2023
@nmadan @rohangujarathi
@Zhankuil @svia3 @stacicho @edwardps @benieric
feature: Add Pipeline step decorator, NotebookJobStep, and scheduler (#…
b380eb3 
…1271)

Co-authored-by: Rohan Gujarathi <[email protected]>
Co-authored-by: svia3 <[email protected]>
Co-authored-by: Zhankui Lu <[email protected]>
Co-authored-by: Dewen Qi <[email protected]>
Co-authored-by: Edward Sun <[email protected]>
Co-authored-by: Stephen Via <[email protected]>
Co-authored-by: Namrata Madan <[email protected]>
Co-authored-by: Stacia Choe <[email protected]>
Co-authored-by: Edward Sun <[email protected]>
Co-authored-by: Edward Sun <[email protected]>
Co-authored-by: Rohan Gujarathi <[email protected]>
fix: Multiple bug fixes including removing unsupported feature. (#1105)
Fix some problems with pipeline compilation (#1125)
fix: Refactor JsonGet s3 URI and add serialize_output_to_json flag (#1164)
fix: invoke_function circular import (#1262)
fix: pylint (#1264)
fix: Add logging for docker build failures (#1267)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laurenyu laurenyu laurenyu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@knakad @sagemaker-bot @laurenyu