Change How App Templates Are Updated for sam init
#1678
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Possible challenge - no escape hatch to NOT update if not desired. Could be worth a larger refactor.
|
Hold on this PR, going for a deeper approach. |
Unit and integ tests still need to be updated, but this change allows us to drop the prompt to update app templates, and will safely handle edge cases that previously required us to prompt on, for example, updated.
Separates into two logical submethods. Still need to completely overhaul init unit and integ tests as a result of the change to the user flow.
…wood45/aws-sam-cli into update-in-non-interactive-init
Also, black formatting updates.
New tests cover most of this ground, and integ tests definitely do. Removing tests with side effects.
Too long for linter
awood45
changed the title
sam init
sriram-mv
suggested changes
Dec 19, 2019
| ) | ||
| # Now we need to delete the old repo and move this one. | ||
| try: |
There was a problem hiding this comment.
Suggestion: can we move this try except block which is inside another try except block to another method, helps with readability.
| click.echo("WARN: Could not clone app template repo.") | ||
|
|
||
| def _clone_new_app_templates(self, shared_dir, expected_path): | ||
| with osutils.mkdir_temp(ignore_errors=True) as tempdir: |
There was a problem hiding this comment.
if we are using ignore_errors, i also suggest we add a callback for onerror to log a debug message.
| self.clone_attempted = True | ||
|
|
||
| def _template_directory_exists(self, expected_path): | ||
| path = Path(expected_path) |
There was a problem hiding this comment.
nit/non-blocking: I notice we are using both pathlib and os.path, is it possible to standardize on one or other.
sriram-mv
approved these changes
Dec 20, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This was initially triggered by a need to support silent updates for the IDEs using
--no-interactiveas an option for init. To properly handle that, while still handling cases where a user might have issues copying the new init templates down for some reason (internet connections, possibly file permissions), this PR now changes our general approach for copying app templates.Now, we use a sort of state machine logic - we use a temp directory to stage the
git cloneof the app templates repo, deleting and copying over the new repo as appropriate to the AWS SAM shared directory. If the clone fails, no change is made and the existing app templates copy is preserved.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.