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

i/builtin: add registry interface #14113

Merged
merged 2 commits into from
Jun 27, 2024

Conversation

MiguelPires
Copy link
Contributor

Add a registry interface that snaps can use to access a particular registry view.

@MiguelPires MiguelPires added the registry registry related work (previously called aspects) label Jun 21, 2024
Copy link
Member

@olivercalder olivercalder left a 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 :)

interfaces/builtin/all_test.go Outdated Show resolved Hide resolved
run-checks Show resolved Hide resolved
interfaces/builtin/registry.go Outdated Show resolved Hide resolved
@MiguelPires
Copy link
Contributor Author

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, snap/snapctl support, hooks, etc (once this is ready to move out of experimental)

Copy link
Member

@olivercalder olivercalder left a 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.

interfaces/builtin/registry_test.go Outdated Show resolved Hide resolved
registry/registry.go Outdated Show resolved Hide resolved
interfaces/builtin/all_test.go Outdated Show resolved Hide resolved
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.

thank you, some comments, I think we miss some bits in the interface static info

registry/registry.go Outdated Show resolved Hide resolved
interfaces/builtin/registry.go Outdated Show resolved Hide resolved
interfaces/builtin/registry.go Show resolved Hide resolved
interfaces/builtin/registry.go Show resolved Hide resolved
interfaces/builtin/registry.go Show resolved Hide resolved
@ernestl ernestl added the Needs Samuele review Needs a review from Samuele before it can land label Jun 25, 2024
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.

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

role = "manager"
plug.Attrs["role"] = role
if !ok || role == "" {
return fmt.Errorf(`registry plug must have a valid "role" attribute`)
Copy link
Collaborator

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?

Copy link
Contributor Author

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]>
@ernestl ernestl merged commit 283f8ab into snapcore:master Jun 27, 2024
47 of 51 checks passed
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 registry registry related work (previously called aspects)
Projects
None yet
4 participants