Skip to content

ctlcmd: add snapctl get.#1983

Merged
kyrofa merged 4 commits into
canonical:masterfrom
kyrofa:feature/1596629/snapctl_get
Sep 23, 2016
Merged

ctlcmd: add snapctl get.#1983
kyrofa merged 4 commits into
canonical:masterfrom
kyrofa:feature/1596629/snapctl_get

Conversation

@kyrofa
Copy link
Copy Markdown
Contributor

@kyrofa kyrofa commented Sep 23, 2016

Currently snapd supports setting configuration values via snapctl set. This PR makes more progress on LP: #1596629 by adding the ability to obtain configuration values via snapctl get.

Currently snapd supports setting configuration values via `snapctl set`.
Add the ability to obtain configuration values by addding `snapctl get`.

Updates: #1596629

Signed-off-by: Kyle Fazzari <kyle@canonical.com>
Copy link
Copy Markdown
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woot!

Comment thread overlord/hookstate/ctlcmd/get.go Outdated
for _, key := range c.Positional.Keys {
var value interface{}
if err := transaction.Get(c.context().SnapName(), key, &value); err != nil {
return fmt.Errorf("cannot get configuration: %s", err)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this prefix? Isn't the message returned implying that already?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you're right-- removed.


for _, key := range c.Positional.Keys {
var value interface{}
if err := transaction.Get(c.context().SnapName(), key, &value); err != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to lock the transaction, so we know all the values returned are consistent with each other (think of a concurrent set user/pass).

It's okay to address that in a follow up, as typically we won't see concurrency within a single hook.

Copy link
Copy Markdown
Contributor Author

@kyrofa kyrofa Sep 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yeah concurrent access within the same hook was not something I really considered. I think this will solve most use-cases, but we can follow up later with that.

Copy link
Copy Markdown
Contributor

@pedronis pedronis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Signed-off-by: Kyle Fazzari <kyle@canonical.com>
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
@kyrofa kyrofa merged commit 10c5029 into canonical:master Sep 23, 2016
@kyrofa kyrofa deleted the feature/1596629/snapctl_get branch September 23, 2016 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants