-
Notifications
You must be signed in to change notification settings - Fork 41
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
feat(snap): add support for devmode confinement #117
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks reasonable, thanks for the addition. A couple of style (keyword arg) comments as well as a note about not using Optional[bool]
(your thoughts welcome).
Additionally, can we please add an integration test that exercises the devmode=True
code path?
classic: Optional[bool] = False, | ||
devmode: Optional[bool] = False, | ||
dangerous: Optional[bool] = False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these should all be changed to non-optional too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I do agree, the scope of this PR wasn't to refactor the existing code. The other comments were addressed though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair -- thanks.
@benhoyt as for the remark related to the integration tests, I could add a test that installs the sdcore-upf snap (which is only available in devmode), however I'm not convinced this is a great idea as the goal at some point will be to have it available as a strictly confined snap and remove devmode confinement (which will break this existing project). Do you have a suggestion here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now, thanks. We can always clean up those other Optional[bool]
s later.
@benhoyt as for the remark related to the integration tests, I could add a test that installs the sdcore-upf snap (which is only available in devmode), however I'm not convinced this is a great idea as the goal at some point will be to have it available as a strictly confined snap and remove devmode confinement (which will break this existing project). Do you have a suggestion here?
Yeah, that's fair enough. In that case I suggest you just test this code manually against a real Juju (you may have done that already) and report back with results here for the record.
classic: Optional[bool] = False, | ||
devmode: Optional[bool] = False, | ||
dangerous: Optional[bool] = False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair -- thanks.
To speed things up, I've merged this pre-emptively -- if you find anything wonky in your manual testing, we can push up another PR. |
Snaps have 3 confinement levels: strict, classic and devmode. devmode snaps are useful during the development process. Here we add support for installing snaps with the devmode level of confinement.
Rationale
The first reason to add support for devmode is by symmetry with snaps/snapcraft.
devmode
is supported by snapcraft.io, why not support it here?The second reason is pragmatic. The telco team has the SD-Core UPF charm which is only available in devmode. Adding strict confinement for this charm won't be straightforward but we don't want to go through the "classic" review process until we really had a try with strict confinement for it. Until then it will only be available in devmode and we need a way to move ahead with the machine charm.