Feature/cmd stopsignal #159
Conversation
pkg/commands/stopsignal.go
Outdated
| } | ||
| signal := resolvedEnvs[len(resolvedEnvs)-1] | ||
|
|
||
| logrus.Infof("Replacing StopSignal in config with %v", signal) |
There was a problem hiding this comment.
We probably want to make sure the signal is valid before setting it -- looks like Docker does something similar here. You could probably use the same method to check!
pkg/commands/stopsignal.go
Outdated
| logrus.Info("cmd: STOPSIGNAL") | ||
|
|
||
| // resolve possible environment variables | ||
| resolvedEnvs, err := util.ResolveEnvironmentReplacementList([]string{s.cmd.Signal}, config.Env, true) |
There was a problem hiding this comment.
Since we aren't resolving file paths for this command, we should pass in false to this method, so it should look like util.ResolveEnvironmentReplacementList([]string{s.cmd.Signal}, config.Env, false)
pkg/commands/stopsignal.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| signal := resolvedEnvs[len(resolvedEnvs)-1] |
There was a problem hiding this comment.
Would this be the same as resolvedEnvs[0]? Could you add ac omment about only having a single element in the output?
There was a problem hiding this comment.
I took this from other commands but without actually testing it, I assume you are correct. The main question would be use a strict 0 here or spend some more compute power in favor of keeping the code more adaptable to possible future enhancements or changes.
There was a problem hiding this comment.
Checked on that and the slide will always be len 1 - as the input slice will always be len 1 as well.
|
Thanks @chrisz100 ! |
|
Could you remove |
|
Great, thank you! |
priyawadhwa
added
the
area/dockerfile-command
Added stopsignal with environment evaluation.
Reverted changes for shell command as I accidentally pushed them to master and there is another PR open and pending for them.
fixes #16