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

Improve non-interactive behaviour of spin new #2643

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

itowlson
Copy link
Contributor

An alternative approach to the problem identified in #2627. With this PR:

  • In a non-interactive environment (not a terminal), spin new and spin add will:
    • Not prompt for parameters (must be supplied via -value, --values-file, or --accept-defaults
    • Not prompt about generating into a non-empty directory (will fail and error out by default)
    • The implementation uses the established interaction::Silent strategy, which already gets exercised via the unit tests
  • spin new and spin add now support a --allow-overwrite flag which will allow generating into a non-empty directory. In a terminal, this skips the prompt; in a non-terminal, it changes the behaviour.

The --allow-overwrite flag has a --allow-overwrites alias because I kept typing the latter when testing despite having defined the singular form not two minutes before that is why.

The PR has unit tests for the non-interactive case; obviously it's not very practical to includes tests for the interactive case but it seemed to work when I tried it!

Copy link
Collaborator

@rylev rylev left a comment

Choose a reason for hiding this comment

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

Looks good! Have some small nits and thoughts but nothing blocking.

}

#[tokio::test]
async fn can_generate_over_existing_files_if_so_configured() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: there's a lot of set up for these tests that is indentical. Would it make sense to test both scenarios in the same test perhaps by doing the "cookies" test, then deleting the spin.toml, and trying again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there's a lot of shared test setup across the template tests. I'd prefer to keep the individual tests specific, but as a later pass we should refactor the entire test suite to reduce duplication/copy-paste and make the "what is specific about this setup and about the expected results" pop a lot more. So I am going to punt on this for now but I do totally accept your point!

/// If the output directory already contains files, generate the new files into
/// it without confirming, overwriting any existing files with the same names.
#[clap(
long = "allow-overwrite",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not 100% sold on the name as it can somewhat beg the question of what we are allowing the overwrite of, but I can't think of a better alternative so I'm fine with it as is.

@itowlson itowlson merged commit 086ded6 into fermyon:main Jul 15, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants