enable global flags for deploy command #634
Conversation
fmoehler
changed the title
jpalermo
requested review from
bgandon,
a team and
danielfor
and removed request for
a team
bgandon
changed the title
fmoehler
force-pushed
the
add-global-flags-from-deploy-config-in-deploy-go
branch
from
0d38221
to
cfbabbb
Compare
cmd/deploy.go
Outdated
|
|
||
| yaml.Unmarshal([]byte(config.Content), &conf) | ||
| if conf.IncludeDeployments == nil || includeContains(conf.IncludeDeployments, c.deployment.Name()) { | ||
| c.ui.PrintLinef("Using deployment flags from config of type '%s' (name: '%s')", config.Type, config.Name) |
There was a problem hiding this comment.
Can we make that information more prominent in the cli logs (e.g. color it, or add !!! a line before and afterwards) otherwise it can be overseen to easy.
Additionally maybe the better message would be:
NOTE: The deploy command execution will use parameters that are globally configured in the director's %s config.
There was a problem hiding this comment.
Sure, we can make it more prominent. I tried to have the text similar to what comes up in the deploy command anyway e.g.
Using deployment 'foo'
So I thought it would be nice to stick to the same sentence "structure"
|
Documentation PR: cloudfoundry/docs-bosh#811 |
fmoehler
force-pushed
the
add-global-flags-from-deploy-config-in-deploy-go
branch
from
74a3741
to
2e7bb37
Compare
fmoehler
force-pushed
the
add-global-flags-from-deploy-config-in-deploy-go
branch
from
2e7bb37
to
55961e4
Compare
| if conf.ExcludeDeployments != nil && | ||
| conf.IncludeDeployments != nil { | ||
| c.ui.PrintLinef("Ignoring deployment flags from config of type '%s' (name: '%s'). Please use only 'include'- OR 'exclude'-property in the config.", config.Type, config.Name) | ||
| } else { | ||
| if (conf.IncludeDeployments == nil && conf.ExcludeDeployments == nil) || | ||
| deploymentIncluded || | ||
| (!deploymentExcluded && conf.ExcludeDeployments != nil) { |
There was a problem hiding this comment.
I first thought of such pseudo-code instead:
decision=NO
IF includeList == null OR includeList contains deploymentName THEN decision=YES
IF excludeList != null AND excludeList contains deploymentName THEN decision=NO
Then, inspired by the director’s code for addons, here is a better version (see other commend below), which is equivalent:
decision = applies(includeList, deploymentName) && !applies(excludeList, deploymentName)
This is easier to understand, and consistent with what we already have for other include/exclude mechanisms in Bosh.
There was a problem hiding this comment.
@bgandon thanks for the review. I agree that would be easier to understand, however only having the check
decision = applies(includeList, deploymentName) && !applies(excludeList, deploymentName)
would not apply to the following cases, that are currently taking care of:
- No warning/ignoring of the flags in case both include and exclude properties are maintained.
- If the exclude property is empty, it will always be a NO decision (considering that the containsDeployment() -> applies() method gets rewritten to return true if
list == null OR list contains deploymentName)
So I am not sure about that...
There was a problem hiding this comment.
That’s right, my second suggestion is not equivalent to the first one, and i didn’t take into account the ambiguous case where both include and exclude are specified.
The idea with my first suggestion, was to skip the ambiguity and just le the exclude win when both are contradictory concerning a given deployment name.
I think we can go ahead with the code as is.
cmd/deploy.go
Outdated
| (!deploymentExcluded && conf.ExcludeDeployments != nil) { | ||
| c.ui.PrintLinef("!!!!!!!") | ||
| c.ui.PrintLinef("Using deployment flags from config of type '%s' (name: '%s')", config.Type, config.Name) | ||
| c.ui.PrintLinef("!!!!!!!") |
There was a problem hiding this comment.
Message should be improved to be similar to other Bosh CLI outputs and still raise the relevant attention from human operators
cmd/deploy.go
Outdated
| return opts | ||
| } | ||
|
|
||
| func containsDeployment(slice []string, value string) bool { |
There was a problem hiding this comment.
Thinking of the implementation done in Director for addons, I think that applies() would be better name for this function, with this pseudo code as implementation:
list == null OR list contains deploymentName
|
The lint issues are fixed now and I'm going to merge this as discussed during the FI WG meeting. |
|
This was released in 7.5.0 |
see: cloudfoundry/bosh-cli#634 Signed-off-by: Rifa Achrinza <[email protected]>
see: cloudfoundry/bosh-cli#634 Signed-off-by: Rifa Achrinza <[email protected]>
see: cloudfoundry/bosh-cli#634 Signed-off-by: Rifa Achrinza <[email protected]>
see: cloudfoundry/bosh-cli#634 Signed-off-by: Rifa Achrinza <[email protected]>
see: cloudfoundry/bosh-cli#634 Signed-off-by: Rifa Achrinza <[email protected]>
see: cloudfoundry/bosh-cli#634 Signed-off-by: Rifa Achrinza <[email protected]>
see: cloudfoundry/bosh-cli#634 Signed-off-by: Rifa Achrinza <[email protected]>
No description provided.