Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/snap: introduce 'snap quota' command #10197

Merged
merged 5 commits into from
Apr 28, 2021

Conversation

stolowski
Copy link
Contributor

Introduce 'snap quota' command along with client methods with support for creating/updating and querying info for single quota group.

@stolowski stolowski added Needs Samuele review Needs a review from Samuele before it can land quota labels Apr 27, 2021
Copy link
Member

@anonymouse64 anonymouse64 left a comment

Choose a reason for hiding this comment

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

some small comments, but I think the direction is good, thanks for working on this!

I still need to look at the tests before giving final +1

client/quota.go Outdated
GroupName string `json:"group-name"`
Parent string `json:"parent,omitempty"`
Snaps []string `json:"snaps,omitempty"`
MaxMemory int64 `json:"max-memory"`
Copy link
Member

Choose a reason for hiding this comment

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

should this be uint64 (negative memory doesn't have a meaning) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, done, although we have inconsistencies elsewhere in existing helpers, so unfortunately there is now int64/uint64 casting (cleaning the helpers up is probably a material for a followup since it would affect unrelated code).

Copy link
Member

Choose a reason for hiding this comment

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

sounds good, I think cleanups to the helpers would be good but yeah are unnecessary at this point

Comment on lines +69 to +70
if maxMemory == "" && x.Parent == "" && len(x.Positional.Snaps) == 0 {
return x.showQuotaGroupInfo(x.Positional.GroupName)
Copy link
Member

Choose a reason for hiding this comment

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

to be clear, we don't need to handle the case where the group name is empty, since go-flags will handle that case for us since we use required: true, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

cmd/snap/cmd_quota.go Outdated Show resolved Hide resolved
}

func (x *cmdQuota) showQuotaGroupInfo(groupName string) error {
w := Stdout
Copy link
Member

Choose a reason for hiding this comment

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

this should probably be a tabWriter() instead so that things line up in a nice format

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I'm not sure, I tried it and it's doing weird things. I think it's geared towards tabular output with constant number of columns, whereas here the output is yamlish.

Copy link
Member

Choose a reason for hiding this comment

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

well I suppose we can revisit it and fix it later, but tabWriter is used in snap model which is also YAMLish

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll revisit this in a followup after all other PRs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think we can do this later, we should just ensure we capture it somewhere.

cmd/snap/cmd_quota.go Show resolved Hide resolved
Copy link
Collaborator

@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.

thanks, I did a first pass

client/quota.go Show resolved Hide resolved
client/quota.go Outdated Show resolved Hide resolved
client/quota.go Outdated Show resolved Hide resolved
client/quota.go Outdated Show resolved Hide resolved
cmd/snap/cmd_quota.go Outdated Show resolved Hide resolved
cmd/snap/cmd_quota.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

thanks, a comment about the TODO/open questions about the command

cmd/snap/cmd_quota.go Show resolved Hide resolved
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Looks good, let's merge and do the rest as followups

}

func (x *cmdQuota) showQuotaGroupInfo(groupName string) error {
w := Stdout
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think we can do this later, we should just ensure we capture it somewhere.

if groupName == "" {
return xerrors.Errorf("cannot create or update quota group without a name")
}
// TODO: use naming.ValidateQuotaGroup()
Copy link
Contributor

Choose a reason for hiding this comment

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

(nitpick^2) given that the server side will validate I'm not sure we really need to client side validate but I'm happy to keep this todo

@mvo5 mvo5 merged commit 687d942 into snapcore:master Apr 28, 2021
pedronis added a commit that referenced this pull request May 27, 2021
Merge pull request #10200 from mvo5/fde-hooks-v2-bind-to-model-v2

The model checker runs after the device is opened. However if this
check fails we want to make sure to close the device again.

The PR also contains a commit with typo/description fixes from the review for #10197
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Samuele review Needs a review from Samuele before it can land
Projects
None yet
4 participants