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

Default install and upgrade to "Y" when no answer is given #1673

Merged
merged 5 commits into from
Nov 10, 2022

Conversation

t-eckert
Copy link
Contributor

@t-eckert t-eckert commented Nov 2, 2022

Changes proposed in this PR:

  • When a user is promped to install Consul, if they give no answer (e.g. just hit enter) the install will occur.

How I've tested this PR:

  • Manually by running an install.

How I expect reviewers to test this PR:

  • You may also run an install.

Checklist:

  • Tests added
  • CHANGELOG entry added

    HashiCorp engineers only, community PRs should not add a changelog entry.
    Entries should use present tense (e.g. Add support for...)

@t-eckert t-eckert marked this pull request as ready for review November 2, 2022 18:52
@t-eckert t-eckert requested review from a team, jmurret and wilkermichael and removed request for a team November 2, 2022 18:52
Copy link
Contributor

@wilkermichael wilkermichael 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! very minor comment

cli/helm/install.go Show resolved Hide resolved
Copy link
Member

@jmurret jmurret left a comment

Choose a reason for hiding this comment

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

Should uninstall and upgrade follow the same paradigm of "you typed this command so I am going to default to you wanted to do this" or are we leaving those with N as the default because there is more risk to those?

@wilkermichael
Copy link
Contributor

Should uninstall and upgrade follow the same paradigm of "you typed this command so I am going to default to you wanted to do this" or are we leaving those with N as the default because there is more risk to those?

If the others switch behaviour, suggest baking logic into the Abort method and adding a unit test for the exported method

@t-eckert
Copy link
Contributor Author

t-eckert commented Nov 2, 2022

Should uninstall and upgrade follow the same paradigm of "you typed this command so I am going to default to you wanted to do this" or are we leaving those with N as the default because there is more risk to those?

I could see doing this for upgrade, but the convention is to only auto-yes for "additive" commands. I wouldn't do it for uninstall because it could be a footgun.

@t-eckert t-eckert requested a review from jmurret November 3, 2022 18:33
@t-eckert t-eckert changed the title Default install to "Y" when no answer is given Default install and upgrade to "Y" when no answer is given Nov 7, 2022
@david-yu
Copy link
Contributor

david-yu commented Nov 9, 2022

Still waiting on @jmurret to review.

Copy link
Member

@jmurret jmurret 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! 🙏

@t-eckert t-eckert merged commit 3106d36 into main Nov 10, 2022
@t-eckert t-eckert deleted the te/default-y-on-install branch November 10, 2022 18:30
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.

4 participants