-
Notifications
You must be signed in to change notification settings - Fork 76
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
Add cli config flag #78
Add cli config flag #78
Conversation
@stilvoid Ping :). Would it be possible for you to have a look at these changes and post your thoughts? |
Hey, thanks so much for this and apologies for the delay in getting back to you! I like the approach and this definitely fits what I had in mind but I have a few minor changes I'd suggest which I'll put into a review on this PR in a moment. |
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.
Thanks again for the contribution!
internal/cmd/deploy/deploy.go
Outdated
} | ||
|
||
for k, v := range cliInput.Parameters { | ||
parsedParams[k] = v |
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.
This means that tags and parameters specified in a config file will override tags and parameters supplied by --tags
or --params
which seems counter-intuitive.
Perhaps we should warn (or fail) on clashing tags/param keys?
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.
I changed the priority. The cli flags tag
and param
can now override the config file. When they do so, a warning will be printed. Is this what you had in mind?
2577b92
to
1647974
Compare
1647974
to
b9680a4
Compare
Thanks for the feedback. I just pushed new changes to address your comments. |
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
*Issue #11
Description of changes: Adds support for parameters/tags as a json file for the deploy command. See the issue for more details on why this may be useful.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.