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

feat: Add support for negative default in confirm prompt, fixes #6103 #6118

Conversation

cballenar
Copy link
Contributor

@cballenar cballenar commented Apr 19, 2024

The Issue

Sensitive operations such as ddev provider push recommend being careful but default to a 'yes' value if left blank.

How This PR Solves The Issue

We add a utility, ConfirmTo(prompt, default) which takes the logic of Confirm(prompt) and allows passing a default value (of true or false).

We then use ConfirmTo in the push command and default to false.

Lastly, Confirm is updated to be a wrapper of ConfirmTo, thus maintaining current behavior without repeating code.

Manual Testing Instructions

  1. Config a project with a provider (any)
  2. Attempt to push to the provider
  3. You should see the prompt message ending in (y/N) and if blank is submitted it will cancel the operation

Automated Testing Overview

Replaces TestConfirm with TestConfirmTo. Since Confirm is just a wrapper, its possible values have already been accounted for in TestConfirmTo.

Related Issue Link(s)

#6103

Release/Deployment Notes

- Confirm is updated to a wrapper of ConfirmTo to preserve current behavior
- Updates comments accordingly
@rfay
Copy link
Member

rfay commented Apr 19, 2024

Thanks!

Since Confirm is just a wrapper, its possible values have already been accounted for in TestConfirmTo.
@cballenar cballenar marked this pull request as ready for review April 20, 2024 10:47
@cballenar cballenar requested a review from a team as a code owner April 20, 2024 10:47
@cballenar
Copy link
Contributor Author

I've added the test for the new function and this should be ready for review.

I wasn't too happy with the name of the function. My initial thought was to simply make the default value optional in the same Confirm function but that's not a thing in go so I had to go this route.

Happy to get some thoughts or feedback!

@stasadev
Copy link
Member

@cballenar, glad you're working on it, I've run tests and it needs formatting.

Please install golangci-lint locally and run this in the project root:

make golangci-lint

@cballenar
Copy link
Contributor Author

Thanks @stasadev ! Linting issues have been resolved now.

@rfay
Copy link
Member

rfay commented Apr 24, 2024

Will eventually get caught up to review, thanks for your patience!

@rfay rfay requested a review from stasadev April 24, 2024 20:11
Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

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

Thank you @cballenar, especially for the elegant refactoring for the tests!
I checked ddev push manually, the default answer is no.

@rfay rfay added this to the v1.23.1 milestone May 3, 2024
Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

This works great, thanks so much, and thanks for your patience!

pkg/util/prompt.go Outdated Show resolved Hide resolved
@rfay rfay merged commit cbe6bc4 into ddev:master May 16, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants