Prompt user before overwriting password #350
Conversation
|
Test failure is from missing dependency. Looks like make deps isn't ran before any tests. Probably good to keep that manual anyway to vet each addition. |
Gopkg.toml
Outdated
|
|
||
| [[constraint]] | ||
| branch = "master" | ||
| name = "github.com/bobappleyard/readline" |
There was a problem hiding this comment.
I'm concerned about this library's dependency on readline. Couple simpler alternatives to consider?
There was a problem hiding this comment.
I'm not tied to this package at all. Readline looks good, I'll swap it in instead
workflows/database_upsert.go
Outdated
| @@ -108,6 +110,14 @@ func (workflow *databaseWorkflow) databaseDeployer(namespace string, service *co | |||
| dbPassVersion, err := paramManager.ParamVersion(dbPassSSMParam) | |||
| if err != nil { | |||
| log.Warningf("Error with ParamVersion for DatabaseMasterPassword, assuming empty: %s", err) | |||
| answer, err := workflow.cliExtension.Prompt("Error retrieving DatabaseMasterPassword. Set a new DatabaseMasterPassword", false) | |||
There was a problem hiding this comment.
This will cause issues for pipelines. The first time the pipeline runs, the prompt will fail since it is running without a TTY attached. Maybe just check if there is a TTY before prompting, othewise assume it's ok?
Check out something like what was done for prompting for github password on mu pipeline up...makes testing much easier: https://github.com/stelligent/mu/blob/develop/cli/pipelines.go#L73
There was a problem hiding this comment.
Ah, I wasn't aware this was called without a TTY attached at any point. I will make sure to handle that.
I did see how you handled pipeline up. I avoided that pattern because of where the prompt is happening (deep in the upsert command). But, maybe I need to consider a larger refactor of this method to move the param checking outside of this method entirely and attach it somewhere else. I'll revisit what I did here and see if I can simplify or abstract it
Had to test that the process would crash with status code 126 if the user did not want to overwrite the password. See https://talks.golang.org/2014/testing.slide#23 for more info the pattern
Retrieving the password param name and version to use is not in it's own step and the prompt is injected into the method for easier testing and less overloading of the underlying workflow object making the code easier to reason about.
readline was added to handle prompt for override password in databaseUpsert. The same library can do what gopass was doing. Removed gopass and used readline instead.
|
@cplee I replaced the readline dependency with https://github.com/tcnksm/go-input and replaced gopass with the same library so we're not just adding more dependencies that can handle the same functionality. I've also refactored databaseDeployer to remove password checking entirely and added it instead to the pipeline executor as it's own step before the deployer runs and updated tests. This change really simplified the logic for tests as I can now inject the prompt struct directly as a param - it also removed one parameter from Deployer - making it simpler to invoke. The one thing I don't know that I addressed is your original TTY question. Just to be clear, the prompt only happens if there is an error contacting SSM. If the parameter isn't set it simply generates a new password, saves it, and moves on. If the user wants they can also override the parameter and version to use in mu.yml in the form of |
|
closes #344 |
If getting the version of the database param fails the user is prompted to accept or deny attempting to create a new password.
I had some issues trying to mock this user input. I eventually settled on adding a new property to the databaseWorkflow struct that is a cliExtension interface then extending that in the tests.
I also pulled in a new package to handle the cli input. I started to just write it but I found a small enough package and noticed there's another one specifically for capturing the github token in the pipeline command. I also considered finding a more robust cli tool (shame the main cli tool doesn't do this) to handle the password and prompt but stopped short because it didn't feel necessary.