Skip to content
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.

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

fix: respect --resolve-s3 and --resolve-image-repos in a guided context. #4873

Merged
merged 2 commits into from Mar 21, 2023

Conversation

sriram-mv
Copy link
Contributor

@sriram-mv sriram-mv commented Mar 15, 2023

Which issue(s) does this change fix?

#4841

Why is this change necessary?

  • This change is necessary so that users can run sam deploy --guided and continue to run sam deploy afterwards without running into issues where --resolve-s3 and --s3-bucket are both set.

How does it address the issue?

  • With this change, whenever a user goes through a guided flow, the resources that are required for deployment are created automatically and the config file updated with --resolve-s3 and --resolve-image-repos

What side effects does this change have?

  • Not really a side effect as this is how it used to work before as well.

sam deploy --guided --s3-bucket <> or sam deploy --guided --image-repositories <> will ignore the bucket or the image repositories as it is the managed flow.

Mandatory Checklist

PRs will only be reviewed after checklist is complete

  • Add input/output type hints to new functions/methods
  • Write design document if needed (Do I need to write a design document?)
  • Write/update unit tests
  • Write/update integration tests
  • Write/update functional tests if needed
  • make pr passes
  • make update-reproducible-reqs if dependencies were changed
  • Write documentation

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

@sriram-mv
Copy link
Contributor Author

sriram-mv commented Mar 15, 2023

Tests fixed.

@github-actions github-actions bot added area/deploy sam deploy command pr/internal labels Mar 16, 2023
* especially when --resolve-s3 or --resolve-image-repos is set, no
  further questions are asked on setting up infrastructure.
@sriram-mv sriram-mv changed the title fix: Do not set s3_bucket explicitly on sam deploy --guided. fix: respect --resolve-s3 and --resolve-image-repos in a guided context. Mar 16, 2023
@sriram-mv sriram-mv marked this pull request as ready for review March 16, 2023 06:06
@sriram-mv sriram-mv requested a review from a team as a code owner March 16, 2023 06:06
self.guided_image_repositories = image_repositories
# NOTE(sriram-mv): The resultant s3 bucket is ALWAYS the managed_s3_bucket. There is no user flow to set it
# within guided.
self.resolve_s3 = True if self.guided_s3_bucket else False
Copy link
Contributor

Choose a reason for hiding this comment

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

Very much a nit comment and might be more on the side of personal preference 😄

Suggested change
self.resolve_s3 = True if self.guided_s3_bucket else False
self.resolve_s3 = bool(self.guided_s3_bucket)

Copy link
Contributor

@mndeveci mndeveci left a comment

Choose a reason for hiding this comment

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

LGTM! Do we need to make sure integration test are succeeding, since this might change the output and input prompts there?

@sriram-mv
Copy link
Contributor Author

@mndeveci : I ran all the deploy integration tests locally and they succeeded.

@hawflau hawflau added this pull request to the merge queue Mar 20, 2023
@hawflau hawflau removed this pull request from the merge queue due to a manual request Mar 20, 2023
@hawflau hawflau added this pull request to the merge queue Mar 20, 2023
@hawflau hawflau merged commit 917926e into aws:develop Mar 21, 2023
65 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/deploy sam deploy command pr/internal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants