Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add status-set, container-spec-set, config-get #8163
Conversation
| + "gopkg.in/yaml.v2" | ||
| +) | ||
| + | ||
| +// ContainerspecSetCommand implements the status-set command. |
| +` | ||
| + return &cmd.Info{ | ||
| + Name: "container-spec-set", | ||
| + Args: "<spec yaml> [<unit-name>]", |
axw
Dec 1, 2017
Member
seems pretty unlikely that a charm will be able to set the spec as YAML as a positional argument
maybe accept a file with -f, and have it default to stdin?
| +` | ||
| + return &cmd.Info{ | ||
| + Name: "container-spec-set", | ||
| + Args: "<spec yaml> [<unit-name>]", |
axw
Dec 1, 2017
Member
make unit name a flag, since it's optional? similar to how we have a flag for specifying a relation for other commands
| + rawYaml, err := ioutil.ReadAll(file) | ||
| + if err != nil { | ||
| + return "", errors.Trace(err) | ||
| + } |
axw
Dec 1, 2017
Member
I think you can replace all of this with
rawYaml, err := s.specFile.Read(ctx)
| + return "", errors.Trace(err) | ||
| + } | ||
| + if len(rawYaml) == 0 { | ||
| + return "", errors.New("container spec file required") |
axw
Dec 1, 2017
Member
I think we could be more helpful:
no container spec specified: pipe container spec to command, or specify a file with --file
?
| + return "", errors.New("container spec file required") | ||
| + } | ||
| + | ||
| + var data map[string]interface{} |
axw
Dec 1, 2017
Member
Sorry I missed this before - why are we mandating YAML here? If we even need it at all, it would be better to put it in the SetContainerSpec method, as potentially other things might want to set the spec.
wallyworld
Dec 1, 2017
Owner
We generally try and do such syntactic validation close to the source, but yeah, probably no need to mandate a format. Later we should add provider specific validation.
|
$$merge$$ |
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
wallyworld commentedDec 1, 2017
Description of change
Copy across status-set and config-get from unit.
Modify status-set to only set application status.
Add container-spec-set hook tool.
QA steps
run the unit tests