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.

Sign up for GitHub

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

Jump to bottom

systemd: provide more detailed errors for unimplemented method in emulation mode #11150

Merged
merged 3 commits into from Dec 7, 2021
Merged

systemd: provide more detailed errors for unimplemented method in emulation mode #11150

merged 3 commits into from Dec 7, 2021

Conversation

stolowski
Copy link
Contributor

@stolowski stolowski commented Dec 6, 2021 •
edited

Provide more details with notImplementedError error to make it easier to understand what goes wrong.

@stolowski stolowski added the Simple 😃 A small PR which can be reviewed quickly label Dec 6, 2021
@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2021 •
edited

Codecov Report

Merging #11150 (7cd87b7) into master (672b1c8) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11150      +/-   ##
==========================================
- Coverage   78.25%   78.24%   -0.01%     
==========================================
  Files         917      917              
  Lines      104205   104207       +2     
==========================================
- Hits        81546    81538       -8     
- Misses      17553    17561       +8     
- Partials     5106     5108       +2
Flag Coverage Δ
unittests 78.24% <0.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
systemd/emulation.go 40.54% <0.00%> (-0.75%) ⬇️
overlord/hookstate/hookmgr.go 74.67% <0.00%> (-0.65%) ⬇️
daemon/api_connections.go 93.04% <0.00%> (-0.54%) ⬇️
overlord/ifacestate/helpers.go 76.96% <0.00%> (-0.49%) ⬇️
cmd/snap/cmd_debug_state.go 70.18% <0.00%> (-0.46%) ⬇️
overlord/ifacestate/handlers.go 64.87% <0.00%> (-0.15%) ⬇️
store/cache.go 71.15% <0.00%> (+1.92%) ⬆️

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 672b1c8...7cd87b7. Read the comment docs.

mvo5
mvo5 reviewed Dec 6, 2021
View reviewed changes
Copy link
Collaborator

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

Thanks, this looks very useful. One naming bikeshed/nitpick.

systemd/emulation.go Outdated
@@ -39,18 +38,24 @@ type emulation struct {
rootDir string
}

var errNotImplemented = errors.New("not implemented in emulation mode")
type errNotImplemented struct {
Copy link
Collaborator

@mvo5 mvo5 Dec 6, 2021 •
edited

Choose a reason for hiding this comment

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

(nitpick) I would now call it notImplementedError, the pattern in go seems to be that the [eE]rrXXXX prefix is used for simple errors like ErrWriteToConnected = errors.New(...) and the xxxxError postfix when there are error structs. I vaguely remember that this was also written down somewhere but I can't find it anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@stolowski stolowski force-pushed the systemd-emulation-not-implemented-error branch from 8d91598 to 347dcd4 Compare December 7, 2021 08:31
@stolowski stolowski requested a review from mvo5 December 7, 2021 08:32
MiguelPires
MiguelPires approved these changes Dec 7, 2021
View reviewed changes
Copy link
Contributor

@MiguelPires MiguelPires left a comment

Choose a reason for hiding this comment

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

LGTM, there's just one tiny typo

systemd/emulation.go Outdated Show resolved Hide resolved
@stolowski @MiguelPires
Fix typo
7cd87b7 
Co-authored-by: Miguel Pires <miguelpires94@gmail.com>
mardy
mardy approved these changes Dec 7, 2021
View reviewed changes
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!

mvo5
mvo5 approved these changes Dec 7, 2021
View reviewed changes
Copy link
Collaborator

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

\o/

@mvo5 mvo5 merged commit 010933a into snapcore:master Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mardy mardy mardy approved these changes

@mvo5 mvo5 mvo5 approved these changes

@MiguelPires MiguelPires MiguelPires approved these changes

Assignees
No one assigned
Labels
Simple 😃 A small PR which can be reviewed quickly
Projects
None yet
Milestone
No milestone
5 participants
@stolowski @codecov-commenter @mardy @mvo5 @MiguelPires