add support for mandatory variables to stack deploy #893
Conversation
thaJeztah
added
status/2-code-review
impact/compose-file
and removed
status/0-triage
labels
|
Linter is complaining; |
|
Otherwise, change LGTM; tested this, and looks to work as expected |
cli/compose/template/template.go
Outdated
| name, errorMessage := partition(substitution, ":?") | ||
| value, ok := mapping(name) | ||
| if !ok || value == "" { | ||
| err = &InvalidTemplateError{Template: errorMessage} |
There was a problem hiding this comment.
This message should include something about "missing value" or "value required"
| for _, tc := range testCases { | ||
| _, err := Substitute(tc.template, defaultMapping) | ||
| assert.Error(t, err) | ||
| assert.IsType(t, &InvalidTemplateError{tc.expectedError}, err) |
There was a problem hiding this comment.
This should check the error message as well as the type.
There was a problem hiding this comment.
Thank you for the review. I have updated the PR. Also removed the tc.expectedError from the InvalidTemplateError as it seemed unnecessary.
Codecov Report
@@ Coverage Diff @@
## master #893 +/- ##
==========================================
+ Coverage 53.26% 53.32% +0.06%
==========================================
Files 258 258
Lines 16357 16374 +17
==========================================
+ Hits 8712 8731 +19
+ Misses 7081 7079 -2
Partials 564 564 |
|
The commit I pushed should fix the lint error |
| }{ | ||
| { | ||
| template: "not ok ${UNSET_VAR:?Mandatory Variable Unset}", | ||
| expectedError: "Mandatory Variable Unset", |
There was a problem hiding this comment.
I don't understand this test case. Why would anyone put a default if the value is required?
Edit: Oh I see, apparently that's how it's supposed to work.
The error message should contain something like "required variable UNSET_VAR is missing a value: Mandatory Variable Unset"
There was a problem hiding this comment.
For reference; this is what it looks like for /bin/sh and Bash:
echo ${UNSET_VAR:?Mandatory Variable Unset}
sh: UNSET_VAR: Mandatory Variable Unset
echo ${UNSET_VAR:?Mandatory Variable Unset}
bash: UNSET_VAR: Mandatory Variable Unset| }, | ||
| { | ||
| template: "not ok ${UNSET_VAR:?}", | ||
| expectedError: "", |
There was a problem hiding this comment.
The error here should be something like "required variable UNSET_VAR is missing a value"
There was a problem hiding this comment.
Sure. I am assuming you are referring to the error message in the test cases here, not changing the Substitute function, is that correct?
There was a problem hiding this comment.
No, I think the error from Substitute should be something like this:
err = &InvalidTemplateError{
Template: fmt.Sprintf("required variable %s is missing a value: %s", name, errorMessage),
}For reference, this is the error in docker-compose: https://github.com/docker/compose/pull/5531/files#diff-b2372f2a81cc5911e04cc7df7d68675bR65
There was a problem hiding this comment.
Thanks for the clarification. I updated the PR (the reg. exp. also needed to be fixed to cover empty error messages)
Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>
Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>
adshmh
force-pushed
the
add-support-for-mandatory-variables-syntax-to-stack-deploy
branch
from
ac2331f
to
5d8ce59
Compare
|
Docs were added in docker/docs#5826 |
This PR adds support for mandatory variables syntax to stack deploy. Fixes #805
Signed-off-by: Arash Deshmeh adeshmeh@ca.ibm.com
- What I did
Added support for mandatory variables syntax, i.e. ${VAR?error message} and ${VAR:?error message}, to stack deploy command.
- How I did it
Added the code required to handle the syntax in cli/compose/template/template.go, and the corresponding tests in cli/compose/template/template_test.go
- How to verify it
Run stack deploy command on a compose file that contains mandatory variable(s).
- Description for the changelog
Added support for mandatory variables to stack deploy command.
- A picture of a cute animal (not mandatory but encouraged)