Interactive Init #1454

awood45 · 2019-10-09T19:20:20Z

This is not quite ready to merge yet - I have tested code paths manually, but there is an expansion of unit and integration testing coverage required to provide a sufficient regression suite.

However, the code proper should be ready to review in tandem with these efforts.

IMPORTANT NOTE: As implemented, this depends on the Python 2.7 deprecation being complete.

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

This is populating new commands and changes some of the relevant documentation. This is very much a WIP and not ready to merge.

This actually covers all values we need to prompt for, but is in dire need of a refactoring, and the GitHub clone logic needs to be implemented.

The flow works, most edge cases covered. Still have some development features that need to be pulled out/resolved.

Handles most cases, still need to clean out some debug logic and write full test suite.

More tests needed to meet the coverage bar since there's a lot of new code, but the existing tests work.

With dependency-manager required, cannot have go showing a None type for that field.

Primarily, refactoring the different primary interactive threads into different methods.

…o interactive-init

sriram-mv

Will do a more detailed pass.

sriram-mv · 2019-10-09T22:20:54Z

samcli/commands/init/__init__.py

+    auto_clone=True,
+):
+    # check for mutually exclusive parameters
+    if location and app_template:


can potentially lift exception cases into decorators on do_cli

That turned out to be a bit problematic when I tried it, as we don't always move to failure - rather, in some cases where we have missing required parameters we move to interactive, in other cases we fail. It's difficult to lift a partial of this out without making the code look more confusing to me.

sriram-mv · 2019-10-09T22:30:12Z

samcli/commands/init/init_templates.py

+        expected_path = os.path.normpath(os.path.join(shared_dir, self._repo_name))
+        if self._should_clone_repo(expected_path):
+            try:
+                subprocess.check_output(["git", "clone", self._repo_url], cwd=shared_dir, stderr=subprocess.STDOUT)


on windows you might need to look for git.exe or git.cmd, Not sure which.

Good callout, adding an investigation item on this.

Took a modified approach based on what we do for AWS CLI. On the one hand, we may want to abstract that to a utility, on the other hand, AWS CLI dependency is going away anyways.

sriram-mv · 2019-10-09T22:34:48Z

samcli/commands/init/__init__.py

+        error_msg = """
+ERROR: Missing required parameters, with --no-interactive set.
+
+Must provide one of the following required parameter combinations:


this is not clear on what constitues a combination, I would put valid combinations in a list, so that its easy to grok for the user.

There are only two, what format did you suggest?

[--name--runtime , --dependency-manager, --app-template] or [--location]. Just having all the ands together in --name and --runtime and --dependency-manager and --app-template made it difficult for me to spot the combination.

sriram-mv · 2019-10-09T22:36:24Z

samcli/commands/init/init_templates.py

+        options = self.init_options(runtime, dependency_manager)
+        choices = map(str, range(1, len(options) + 1))
+        choice_num = 1
+        for o in options:


for option in options, instead of o

sriram-mv · 2019-10-09T22:42:09Z

samcli/commands/init/init_templates.py

+            if self._no_interactive:
+                return self._auto_clone
+            do_clone = click.confirm(
+                "This process will clone app templates from https://github.com/awslabs/aws-sam-cli-app-templates - is this ok?"


do we prompt even in a non interactive mode here?

ah I misread the indentation.

sriram-mv · 2019-10-09T22:45:08Z

samcli/commands/init/interactive_init_flow.py

+
+
+def do_interactive(location, runtime, dependency_manager, output_dir, name, app_template, no_input):
+    if location:


Can we convert if elses into: dictionary dispatch tables? https://medium.com/better-programming/dispatch-tables-in-python-d37bcc443b0b

Windows compatibility check, essentially, and this commit also includes a Black reformatting.

This enables us to properly test prompts and improves code coverage. We also managed to entirely remove a code path (location cannot currently be missing when going to interactive). Remaining work is to improve unit test coverage of init templates.

It was incorrect to use 'takewhile', as it terminates as soon as there is a failed check. For Java Maven this caused no templates to be usable.

Now over the 95% threshold, so `make pr` is passing.

sriram-mv · 2019-10-15T21:13:05Z

samcli/commands/init/__init__.py

 @click.option(
    "--no-input",
    is_flag=True,
    default=False,
-    help="Disable prompting and accept default values defined template config",
+    help="Disable Cookiecutter prompting and accept default values defined template config",


can you be more verbose on what constitutes template config?

sriram-mv · 2019-10-15T21:15:05Z

samcli/commands/init/__init__.py

+        error_msg = """
+ERROR: Missing required parameters, with --no-interactive set.
+
+Must provide one of the following required parameter combinations:


[--name--runtime , --dependency-manager, --app-template] or [--location]. Just having all the ands together in --name and --runtime and --dependency-manager and --app-template made it difficult for me to spot the combination.

sriram-mv · 2019-10-15T21:18:06Z

samcli/commands/init/init_templates.py

+                return list(templates_by_dep)
+            return templates
+
+    def _init_options_from_bundle(self, runtime, dependency_manager):


bundle -> bundled

sriram-mv · 2019-10-15T21:20:16Z

samcli/commands/init/init_templates.py

+            if self._no_interactive:
+                return self._auto_clone
+            do_clone = click.confirm(
+                "This process will clone app templates from https://github.com/awslabs/aws-sam-cli-app-templates - is this ok?"


ah I misread the indentation.

sriram-mv · 2019-10-15T21:22:32Z

samcli/commands/init/init_templates.py

+        choices = map(str, range(1, len(options) + 1))
+        choice_num = 1
+        for o in options:
+            if o.get("displayName") is not None:


is displayname part of cookiecutter options? in the cookiecutter.json file. I see it used in the manifest in a test below. Never mind looks to be part of manifest.json

https://github.com/awslabs/aws-sam-cli-app-templates/blob/master/manifest.json

sriram-mv · 2019-10-15T21:31:26Z

samcli/commands/init/init_templates.py

+        for mapping in list(itertools.chain(*(RUNTIME_DEP_TEMPLATE_MAPPING.values()))):
+            if runtime in mapping["runtimes"] or any([r.startswith(runtime) for r in mapping["runtimes"]]):
+                if not dependency_manager or dependency_manager == mapping["dependency_manager"]:
+                    mapping["appTemplate"] = "hello-world"  # when bundled, use this default template name


do we want the logic of falling back to the bundled be driven by manifest in an external repository? https://github.com/awslabs/aws-sam-cli-app-templates/blob/master/manifest.json#L102

I am not sure that's possible. If we get to this piece of code, the manifest is not accessible. Or is this a separate scenario you're thinking about?

No, I was just thinking that falling back to bundled templates (within the same repo), required direction from a manifest file in another repo (app templates repo), introduces some coupling between the repos.

sriram-mv · 2019-10-15T21:32:14Z

samcli/commands/init/init_templates.py

+                    [self._git_executable(), "clone", self._repo_url], cwd=shared_dir, stderr=subprocess.STDOUT
+                )
+                self.repo_path = expected_path
+            except OSError:


do we catch permissions errors as well within OSError?

Per its documentation, I believe that it should.

sriram-mv · 2019-10-15T21:34:54Z

samcli/commands/init/init_templates.py

+            except OSError:
+                click.echo("WARN: Can't clone app repo, git executable not found.")
+            except subprocess.CalledProcessError as clone_error:
+                output = clone_error.output.decode("utf-8")


can we just look at exit codes instead?

This follows the pattern we use for the AWS CLI, what are you proposing?

aah never mind, looks like CalledProcessError actually looks at exit codes natively :)

samcli/commands/init/interactive_init_flow.py

tests/unit/commands/init/test_templates.py

jfuss · 2019-10-28T18:10:56Z

samcli/commands/init/__init__.py

 from samcli.lib.telemetry.metrics import track_command
-from samcli.local.common.runtime_template import INIT_RUNTIMES, SUPPORTED_DEP_MANAGERS, DEFAULT_RUNTIME
+from samcli.commands.init.init_generator import do_generate


Some of these imports should be pushed down into the do_cli function. This is to keep --help text load time low, otherwise we will be loading all of these modules.

Which imports?

This seems to create other problems, we can discuss in private.

Slashes behave differently.

Checking integration test behavior.

Had a bug where the wrong executable name would lead to a premature exit. This change fixes that.

Will better align with errors in place.

This is a WIP to fix breaking Windows integ tests.

This should have been part of the last commit.

If the shared directory doesn't exist, create it before cloning the template git repo. This will need to be refactored as some layer code does the same thing.

Init code assumed existence of shared directory, which is likely but not guaranteed.

awood45 added 11 commits September 13, 2019 15:01

WIP: Interactive Init Command

45e75b6

This is populating new commands and changes some of the relevant documentation. This is very much a WIP and not ready to merge.

WIP: Mutually Exclusive and Required Params

33eb157

WIP: Stubbed Out Interactive Flow

5aba829

This actually covers all values we need to prompt for, but is in dire need of a refactoring, and the GitHub clone logic needs to be implemented.

WIP: Working Flow

01206f1

The flow works, most edge cases covered. Still have some development features that need to be pulled out/resolved.

WIP: Working Init Flow

911dc86

Handles most cases, still need to clean out some debug logic and write full test suite.

WIP: Existing Init Unit Tests Pass

80f8307

More tests needed to meet the coverage bar since there's a lot of new code, but the existing tests work.

WIP: Clean Up Lint Warnings, Add Go Dep Manager

58a6e42

With dependency-manager required, cannot have go showing a None type for that field.

Linting Improvements

ad09dc3

Primarily, refactoring the different primary interactive threads into different methods.

Merge branch 'develop' into interactive-init

6cf331b

Add Docstrings, Fix Branch Indentation

535544b

Merge branch 'interactive-init' of github.com:awood45/aws-sam-cli int…

f4b528b

…o interactive-init

awood45 requested a review from sriram-mv October 9, 2019 19:22

sriram-mv requested changes Oct 9, 2019

View reviewed changes

awood45 added 9 commits October 9, 2019 16:10

Remove Planning Comment

b6d7cf6

Check for Multiple Git Executables

64b45ea

Windows compatibility check, essentially, and this commit also includes a Black reformatting.

Add Click Command Unit Tests for Init

8993087

This enables us to properly test prompts and improves code coverage. We also managed to entirely remove a code path (location cannot currently be missing when going to interactive). Remaining work is to improve unit test coverage of init templates.

Fix _git_executable Function

bf2f2b3

Fix Git Templates Function Call

8c53066

Use Filter for Template Selection

8066ae8

It was incorrect to use 'takewhile', as it terminates as soon as there is a failed check. For Java Maven this caused no templates to be usable.

Unit Test the Template Class

b09929f

Additional Test Coverage

694a0be

Now over the 95% threshold, so `make pr` is passing.

Remove Unused Variable

9eb8167

sriram-mv requested changes Oct 15, 2019

View reviewed changes

awood45 added 7 commits October 15, 2019 15:45

Address Init PR Comments

0378bcc

Update Init Integ Tests

7438d9f

Run of Black Reformatting

2900324

Merge branch 'develop' into interactive-init

c238d82

Merge develop into interactive-init

6df7b0a

Improvements to fix linter issues in init

c4025a3

Update Mock Import

d34ea10

jfuss requested changes Oct 28, 2019

View reviewed changes

awood45 added 14 commits October 28, 2019 12:24

Try batch file for git

7061eb6

Make Unit Test for Init Windows-Compatible

7cf2e35

Slashes behave differently.

Switch execname order

cc3cba0

Checking integration test behavior.

Catch exceptions when checking git executables

5053efa

Had a bug where the wrong executable name would lead to a premature exit. This change fixes that.

Add explicit exception for missing git

0ec8bb2

Will better align with errors in place.

Add git executable unit test

7624b20

Add git executable check

efcc51f

Add Exception Info when Git Fails

4b76385

This is a WIP to fix breaking Windows integ tests.

Fix Warning Method

b3594a0

This should have been part of the last commit.

Run git in Appveyor test stage

3382135

WIP: Create Shared Directory

f8d7977

If the shared directory doesn't exist, create it before cloning the template git repo. This will need to be refactored as some layer code does the same thing.

Create Shared Dir if Necessary

7255c88

Init code assumed existence of shared directory, which is likely but not guaranteed.

Add Shared Dir Unit Tests

957e802

Add One More Test

216b802

awood45 requested review from sriram-mv and jfuss October 29, 2019 17:59

Move Local Imports into do_cli

8fe22cc

jfuss approved these changes Oct 29, 2019

View reviewed changes

sriram-mv approved these changes Oct 29, 2019

View reviewed changes

awood45 merged commit 4d9a3a0 into aws:develop Oct 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interactive Init #1454

Interactive Init #1454

awood45 commented Oct 9, 2019

sriram-mv left a comment

sriram-mv Oct 9, 2019

awood45 Oct 9, 2019

sriram-mv Oct 9, 2019

awood45 Oct 9, 2019

awood45 Oct 10, 2019

sriram-mv Oct 9, 2019

awood45 Oct 15, 2019

sriram-mv Oct 15, 2019

sriram-mv Oct 9, 2019

sriram-mv Oct 9, 2019

awood45 Oct 9, 2019

sriram-mv Oct 15, 2019

sriram-mv Oct 9, 2019

sriram-mv Oct 15, 2019

sriram-mv Oct 15, 2019

sriram-mv Oct 15, 2019

sriram-mv Oct 15, 2019

sriram-mv Oct 15, 2019

sriram-mv Oct 15, 2019

awood45 Oct 15, 2019

sriram-mv Oct 15, 2019

sriram-mv Oct 15, 2019

awood45 Oct 16, 2019 •

edited

Loading

sriram-mv Oct 15, 2019

awood45 Oct 16, 2019

sriram-mv Oct 16, 2019

jfuss Oct 28, 2019

awood45 Oct 28, 2019

awood45 Oct 28, 2019



		def do_interactive(location, runtime, dependency_manager, output_dir, name, app_template, no_input):
		if location:

Interactive Init #1454

Interactive Init #1454

Conversation

awood45 commented Oct 9, 2019

sriram-mv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awood45 Oct 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awood45 Oct 16, 2019 •

edited

Loading