-
Notifications
You must be signed in to change notification settings - Fork 156
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
remove PREFERRED capabilities #1840
base: main
Are you sure you want to change the base?
Conversation
Way back when I added support for core boot classic installs, we did not have a way to indicate whether the installed model preferred to be installed encrypted or unencrypted. Roll time on 9 months or so and our API now supports a list of capabilites and we can define that we can indicate which is preferred by putting it first in the list. One part of the implementation is a bit delicate really -- relying on list.sort() being stable -- so maybe we should clean that up. But I think the other changes make everything nicer.
cc @d-loose for API impact -- we don't have to land this on any particular time frame. but I'm not sure the desktop client will care. |
|
||
DD = enum.auto() | ||
|
||
def __lt__(self, other) -> bool: | ||
if self.is_core_boot() and other.is_core_boot(): | ||
return 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.
So GC.CORE_BOOT_ENCRYPTED and GC.CORE_BOOT_UNENCRYPTED will not compare equal but they will both compare "greater or equal" to each other? Am I getting this right?
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.
Additionally, if we just removed these lines what would be the result? As the sort order is said to matter now and the first one is what we're suggesting as a default, if the only choices were CORE_BOOT_{UN,}ENCRYPTED, which would we want to show first?
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.
if the only choices were CORE_BOOT_{UN,}ENCRYPTED, which would we want to show first
Well that's kind of the point of all this, the model decides. We look at the storage-safety field, which comes from here:
https://github.com/snapcore/snapd/blob/master/asserts/model.go#L362-L377
We interpret prefer-encrypted as "the encryption check box should default on" and prefer-unencrypted as "the check box should default off" (the other fields disable the check box, they are easier). So the answer to your question is "it depends".
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.
Kind of ugly to handle from the enum.
The problem with these lines is that, as previously noted, this produces unstable behavior. A stable sort that is not generalized but correct for the current model is preferable to one that could produce an unpredictable sort order.
We could note the future work here, if that helps.
Way back when I added support for core boot classic installs, we did not have a way to indicate whether the installed model preferred to be installed encrypted or unencrypted. Roll time on 9 months or so and our API now supports a list of capabilites and we can define that we can indicate which is preferred by putting it first in the list.
One part of the implementation is a bit delicate really -- relying on list.sort() being stable -- so maybe we should clean that up. But I think the other changes make everything nicer.