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: rm unnecessary validation #11249

Merged
merged 1 commit into from
Jan 14, 2022
Merged

Conversation

MiguelPires
Copy link
Contributor

This PR removes a validation check for snap install since go-flags's validation already covers it. Apparently, if the positional struct contains a slice, the slice field must also have a required tag. I had a look at the other commands and did some testing to see if this happened anywhere else but didn't see other examples of this quirk.

@MiguelPires MiguelPires added the Simple 😃 A small PR which can be reviewed quickly label Jan 13, 2022
Copy link
Contributor

@stolowski stolowski left a comment

Choose a reason for hiding this comment

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

Thanks for investigating this weirdness!

@@ -480,7 +480,7 @@ type cmdInstall struct {
IgnoreValidation bool `long:"ignore-validation"`
IgnoreRunning bool `long:"ignore-running" hidden:"yes"`
Positional struct {
Snaps []remoteSnapName `positional-arg-name:"<snap>"`
Snaps []remoteSnapName `positional-arg-name:"<snap>" required:"1"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Doh.

@codecov-commenter
Copy link

Codecov Report

Merging #11249 (220298c) into master (2fce820) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11249      +/-   ##
==========================================
- Coverage   78.35%   78.33%   -0.03%     
==========================================
  Files         923      924       +1     
  Lines      105342   105371      +29     
==========================================
+ Hits        82544    82545       +1     
- Misses      17658    17686      +28     
  Partials     5140     5140              
Flag Coverage Δ
unittests 78.33% <ø> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/snap/cmd_snap_op.go 78.30% <ø> (-0.10%) ⬇️
gadget/ondisk.go 67.12% <0.00%> (-6.30%) ⬇️
daemon/api_debug.go 53.13% <0.00%> (-3.82%) ⬇️
cmd/snap/cmd_debug_disks.go 36.36% <0.00%> (ø)
cmd/snap/cmd_debug_state.go 70.64% <0.00%> (+0.45%) ⬆️
interfaces/policy/policy.go 95.26% <0.00%> (+0.52%) ⬆️
osutil/synctree.go 79.24% <0.00%> (+2.83%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fce820...220298c. Read the comment docs.

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.

Nice, thank you!

@mvo5 mvo5 merged commit b6d3fbf into snapcore:master Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Simple 😃 A small PR which can be reviewed quickly
Projects
None yet
4 participants