feat(init): add --checkout option to sam init command for specifying git branch, tag, or commit - #9124
Conversation
…git branch, tag, or commit for cookiecutter projects
- Error when --checkout is used without --location, using the existing ClickMutex required_param_lists mechanism - Add integration test verifying --checkout without --location fails - Add integration test for --checkout with --location (git URL) - Add integration test for --checkout with incompatible params
…anch/tag checkout
…r cookiecutter and non-cookiecutter projects
github-actions
Bot
added
area/init
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
…nd-for-noncookiecutter-projects
![aws-sam-tooling-bot[bot]](https://avatars.githubusercontent.com/b/1290?s=60&v=4){kind=small}
| process.kill() | ||
| raise | ||
|
|
||
| self.assertEqual(process.returncode, 0) |
There was a problem hiding this comment.
[BUG] This test asserts returncode == 0, but the command is invoked without --no-input against a real cookiecutter template. Tracing the path: do_cli only forces no_input = True for the --app-template branch or JSON output, and generate_project only forces it when not location — so here no_input stays False and cookiecutter() will prompt for every key in the template's cookiecutter.json. The subprocess inherits stdin, which is not a TTY under CI, so the prompt hits EOF and click aborts with a non-zero exit (or the test blocks until TIMEOUT). The existing TestInitWithArbitraryProject tests avoid this only because a zip/non-cookiecutter location takes the RepositoryNotFound fallback, which never prompts — there is no existing integration test that clones a cookiecutter git template, so there is no precedent for this assertion holding.
Adding --no-input makes the run deterministic (extra_context already supplies project_name):
"init",
"--location",
"https://github.com/aws-samples/cookiecutter-aws-sam-python.git",
"--checkout",
"improv/cleanup",
"--no-input",
"--name",
"test-checkout-project",Separately, pinning on the improv/cleanup branch of a repo outside this project's control makes the test permanently fragile — if that branch is renamed or deleted the suite breaks. A tag or commit SHA in a repo you own would be more stable.
| tracing, | ||
| application_insights, | ||
| structured_logging, | ||
| checkout=None, |
There was a problem hiding this comment.
[GENERAL] The checkout parameter threaded through do_interactive → generate_application → _generate_from_location/_generate_from_use_case is unreachable with a non-None value from the CLI. --checkout declares required_param_lists=[["location"]], and in do_cli any truthy location takes the if location or zip_bool or image_bool branch, so do_interactive is only ever called with checkout=None.
The one way to reach it is a samconfig.toml entry, since ClickMutex.handle_parse_result returns early when self.name not in opts and config values arrive via ctx.default_map rather than opts — bypassing the --location requirement entirely. In that case _generate_from_use_case overwrites location with templates.location_from_app_template(...) (a local cache path), and the checkout is silently dropped by cookiecutter. Either drop the interactive plumbing, or make the interactive path actually honor/reject checkout rather than silently ignoring it.
| self.assertEqual(process.returncode, 0) | ||
| self.assertTrue(Path(temp, "test-checkout-project").exists()) | ||
| self.assertFalse(Path(temp, "test-checkout-project", "cookiecutter.json").exists()) | ||
| self.assertFalse(Path(temp, "test-checkout-project", ".env").exists()) #This branch does not have .env files, so they should not exist. |
There was a problem hiding this comment.
[GENERAL] Two spots will fail black --check, which make pr runs over tests: the inline comment on this line needs two spaces before # and one space after () # This branch...), and the separator line before the next @pytest.mark.tier2 (around line 1055) contains whitespace on an otherwise blank line. Running make black fixes both. Related: the standalone , placed after # fmt: on in tests/unit/commands/init/test_cli.py:834 is valid but confusing — moving checkout=None above the # fmt: on marker keeps the trailing comma on the preceding argument.
|
Hello @vicheey, I've addressed your feedbacks. Also, the develop branch is merged to the feature branch and resolved all the conflicts. |
| process.kill() | ||
| raise | ||
|
|
||
| self.assertEqual(process.returncode, 0) |
There was a problem hiding this comment.
[BUG] This test asserts returncode == 0 but never passes --no-input, so cookiecutter will prompt and the command cannot succeed non-interactively. Tracing the path with --location --name test-checkout-project:
- do_cli only forces no_input = True in the --app-template branch (if app_template and not location) or for --output json. Neither applies, so no_input stays False.
- generate_project only forces it via if not location and name is not None, and location is set here, so it stays False.
- cookiecutter(no_input=False, ...) prompts for every key in the target template's cookiecutter.json. extra_context ({"project_name": ...}) only changes the shown defaults, it does not suppress the prompts.
With stdin at EOF under pytest the prompt aborts (exit 1); with an attached tty it blocks until TIMEOUT. Either way the assertion fails. Compare test_init_command_output_json_with_template_hook_output, which uses --location without --no-input only because --output json forces no_input = True, and TestInitWithArbitraryProject, whose zip location never reaches cookiecutter's prompt loop.
The same missing --no-input makes test_init_command_checkout_with_location_for_non_cookiecutter_project (line 1055) flaky rather than deterministically broken: cookiecutter caches clones in ~/.cookiecutters/, and clone() calls prompt_and_delete(repo_dir, no_input=False) when that directory already exists, so a second run on the same machine prompts instead of re-cloning.
Separately, the assertion that .env is absent (line 997-999) does not demonstrate that the checkout took effect — it also passes if --checkout were ignored entirely, unless the default branch is known to contain that file. Asserting a file or content unique to improv/cleanup would actually test the new behavior.
| get_sam_command(), | ||
| "init", | ||
| "--location", | ||
| "https://github.com/aws/aws-sam-cli.git", |
There was a problem hiding this comment.
[GENERAL] These tests hard-depend on third-party refs that the SAM CLI team does not control: improv/cleanup on aws-samples/cookiecutter-aws-sam-python and master on aws/aws-sam-cli. If either ref is renamed or deleted the suite breaks with no relation to SAM CLI code.
The aws/aws-sam-cli case is also expensive: cookiecutter's clone() runs a full git clone (no --depth), and this path clones the repository twice — once into ~/.cookiecutters by cookiecutter() before RepositoryNotFound, then again into the temp dir by generate_non_cookiecutter_project. That is a lot of network and disk for an assertion that only checks the output directory exists.
A small fixture repository created in the test with git init / git commit / git branch (as test_init_command_output_json_with_template_hook_output does for its template) would make all three checkout tests hermetic, fast, and able to assert branch-specific content.
3 participants
Which issue(s) does this change fix?
#3555
Why is this change necessary?
Currently,
sam initcannot clone a remote template and check out a specific branch/tag/commit in one step. The template have to be cloned first and then do a local checkout. Only then the specific branch/tag/commit could be utilized to initialize sam project which is not intuitive.How does it address the issue?
The change adds a checkout flag with the
sam initcommand. With the help of that the developers can checkout to their remote url custom branch, tag, or commit utilizing thesam initcommand. Also, added unit tests for checkout and updated the existing unit tests for the new checkout flag.What side effects does this change have?
No side effects.
Mandatory Checklist
PRs will only be reviewed after checklist is complete
make prpassesmake update-reproducible-reqsif dependencies were changedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.