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

asserts, snapstate: return full validation set keys from CheckPresenceRequired and CheckPresenceInvalid #10774

Merged
merged 2 commits into from Sep 17, 2021

Conversation

stolowski
Copy link
Contributor

Return full validation set keys from CheckPresenceRequired and CheckPresenceInvalid. Full keys including series and sequence are expected by store api.

@stolowski stolowski added Needs Samuele review Needs a review from Samuele before it can land validation-sets ✅ labels Sep 14, 2021
@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2021

Codecov Report

Merging #10774 (0793b96) into master (a18b420) will decrease coverage by 0.01%.
The diff coverage is 40.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10774      +/-   ##
==========================================
- Coverage   78.33%   78.32%   -0.02%     
==========================================
  Files         890      890              
  Lines      100198   100205       +7     
==========================================
- Hits        78488    78483       -5     
- Misses      16792    16804      +12     
  Partials     4918     4918              
Flag Coverage Δ
unittests 78.32% <40.00%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
asserts/snapasserts/validation_sets.go 97.24% <40.00%> (-1.82%) ⬇️
daemon/daemon.go 76.40% <0.00%> (-1.52%) ⬇️
cmd/snap/cmd_aliases.go 90.14% <0.00%> (-1.41%) ⬇️
overlord/hookstate/hookmgr.go 74.17% <0.00%> (-0.67%) ⬇️
daemon/api_connections.go 93.04% <0.00%> (-0.54%) ⬇️
overlord/ifacestate/handlers.go 64.92% <0.00%> (+0.14%) ⬆️
interfaces/sorting.go 100.00% <0.00%> (+1.78%) ⬆️
osutil/udev/netlink/rawsockstop.go 71.42% <0.00%> (+4.76%) ⬆️

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 a18b420...0793b96. Read the comment docs.

Copy link
Contributor

@mardy mardy left a comment

Choose a reason for hiding this comment

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

LGTM, but those error returns you added are not covered by unit tests. I'm not sure if they need to be covered -- just pointing that out in case you forgot about them. :-)

@stolowski
Copy link
Contributor Author

stolowski commented Sep 14, 2021

LGTM, but those error returns you added are not covered by unit tests. I'm not sure if they need to be covered -- just pointing that out in case you forgot about them. :-)

No, I didn't forget about them but as with most internal errors, they would be hard/impossible to test because they could only occur if we had a bug somewhere and our structs were not consistent, we usually don't test such cases.

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, small detail about Ref vs At

if vs == nil {
return nil, unspecifiedRevision, fmt.Errorf("internal error: no validation set for %q", rc.validationSetKey)
}
keys = append(keys, strings.Join(vs.At().PrimaryKey, "/"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/At/Ref/ works as well and is more typical for this use case

if vs == nil {
return nil, fmt.Errorf("internal error: no validation set for %q", rc.validationSetKey)
}
keys = append(keys, strings.Join(vs.At().PrimaryKey, "/"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

@stolowski stolowski force-pushed the validation-sets/fix-vset-keys branch 2 times, most recently from 0793b96 to 1332ada Compare September 17, 2021 07:34
@mvo5 mvo5 merged commit bf7a861 into snapcore:master Sep 17, 2021
@stolowski stolowski deleted the validation-sets/fix-vset-keys branch October 20, 2021 09:07
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
5 participants