-
Notifications
You must be signed in to change notification settings - Fork 95
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
fix: run once command list #427
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good to me! I'm a bit surprised at the amount of doc changes that are melded in one commit, there's a lot in there.
Would you be ok to split the PR in two commits? One for the doc improvements, and one for the command fix? That'd be easier to understand the scope of the change here.
Sure, I can happily split it out today. |
Updates RunOnceCommandList to a slice of strings `[]string` and updates 'guiRunOnce()` to pass "" if no commands are provided. This will ensure that the function addresses each case. Ref: #419 Signed-off-by: Ryan Johnson <[email protected]>
10382f1
to
2a31f38
Compare
Simplified in 2a31f38, @lbajolet-hashicorp. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the split!
Posted the other half of the split from the original commit. |
Summary
Updates
RunOnceCommandList
to a slice of strings[]string
and updatesguiRunOnce()
to pass""
if no commands are provided. This will ensure that the function addresses each case.Testing
Basic:
Scenarios:
✅
run_once_command_list
not provided.✅
run_once_command_list []
used.✅
run_once_command_list [""]
used.✅
run_once_command_list ["command1", "command2"]
used.Reference
Closes #419