-
Notifications
You must be signed in to change notification settings - Fork 574
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
i/builtin: add registry interface #14113
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.
Looks good, thanks! I have a few questions. Also, I think in general it might be useful to add a bit more documentation in interfaces/builtin/registry.go
explaining what a registry is, what a view is, and why/how to use them. Often the snapcraft.io documentation doesn't contain much but a link back to the source code for the interface, and in this case I wouldn't (and don't really at the moment) have much/any idea what registries and views are if I were looking through available interfaces :)
Thank you for the suggestion, I've added a small note summarizing how the registry interface can be used. But we'll really need some long form documentation that covers the assertion, interface, |
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, thanks! Nothing blocking, just a few nitpicks and a question.
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.
thank you, some comments, I think we miss some bits in the interface static info
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.
thank you for the changes, I think we should adjust the regexp. Small comment about the role check, also I wonder on something that is not blocker given we are still experimental but we might want to touch on again in our design discussions
interfaces/builtin/registry.go
Outdated
role = "manager" | ||
plug.Attrs["role"] = role | ||
if !ok || role == "" { | ||
return fmt.Errorf(`registry plug must have a valid "role" attribute`) |
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.
should we actually unify this check with the one below? Also thinking further I wonder if actually we should allow this to be left out without default maybe or have a third value?
- manager: snap can access the view and have change-registry etc hooks called for the view
- obsever: snap can access the view and can have registry-changed hooks invoked for the view
emtpy
: snap can only access the view
maybe something to rediscuss?
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.
What would be the use case? If a snap can read the view, then the same level of "access" is implied and it can just have a no-op hook if it doesn't want to be notified.
Add a registry interface that snaps can use to access a particular registry view. Signed-off-by: Miguel Pires <[email protected]>
Signed-off-by: Miguel Pires <[email protected]>
f4ff505
to
499faf0
Compare
Add a registry interface that snaps can use to access a particular registry view.