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

add API for Image Files providers #7802

Merged
merged 1 commit into from
Jun 27, 2024
Merged

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Jun 24, 2024

Signed-off-by: Philippe Martin [email protected]

What does this PR do?

Adds an API for extensions to registrer as an Image Files provider.

Inspiration from spec and Node implementation of Image builder.

What issues does this PR fix or reference?

Part of #7801

@feloy feloy requested review from benoitf and a team as code owners June 24, 2024 13:02
@feloy feloy requested review from cdrage, axel7083 and deboer-tim and removed request for a team June 24, 2024 13:02
@feloy feloy marked this pull request as draft June 24, 2024 13:59
@feloy feloy marked this pull request as ready for review June 24, 2024 15:54
Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

it's still unclear to me:

  • if the namespace is correct (like shouldn't be part of the provider namespace)
  • the metadata argument as we already provide the object (like why it's not part of the object given)
  • if the pattern is register where we provide everything or create (returning an object with helper methods)

packages/extension-api/src/extension-api.d.ts Outdated Show resolved Hide resolved
packages/extension-api/src/extension-api.d.ts Outdated Show resolved Hide resolved
@feloy
Copy link
Contributor Author

feloy commented Jun 25, 2024

it's still unclear to me:

  • if the namespace is correct (like shouldn't be part of the provider namespace)
  • the metadata argument as we already provide the object (like why it's not part of the object given)
  • if the pattern is register where we provide everything or create (returning an object with helper methods)

I wanted to keep the same pattern as for the image checker API

@benoitf
Copy link
Collaborator

benoitf commented Jun 25, 2024

@feloy but why it can't go to the provider namespace vs having to declare a new imageFiles namespace ?

@feloy
Copy link
Contributor Author

feloy commented Jun 25, 2024

@feloy but why it can't go to the provider namespace vs having to declare a new imageFiles namespace ?

I mean that if we move the function to the provider namespace, there will be a specific namespace for the image checker providers imageChecker, but for the image files providers, the function will be in the more generic provider. Also, if we change from register pattern to create, there will be 2 different patterns to register image files vs checkers providers.

I would prefer to be consistent between both providers. Or do you think we should, eventually, move the function from the imageChecker namespace to the providers one?

@benoitf
Copy link
Collaborator

benoitf commented Jun 25, 2024

probably imageChecker should be moved as well

@benoitf
Copy link
Collaborator

benoitf commented Jun 25, 2024

there will be 2 different patterns to register image files vs checkers providers.

we already have 2 patterns for contributions, as they're bringing different kind of value.

create pattern and register pattern

if we have common 'helper functions' in all the implementations, probably it's more following the 'create' pattern

createImageFileProvider(...) : ImageFileProvider
vs
registerImageFileProvider(...): Disposable

@feloy
Copy link
Contributor Author

feloy commented Jun 25, 2024

there will be 2 different patterns to register image files vs checkers providers.

we already have 2 patterns for contributions, as they're bringing different kind of value.

create pattern and register pattern

if we have common 'helper functions' in all the implementations, probably it's more following the 'create' pattern

createImageFileProvider(...) : ImageFileProvider vs registerImageFileProvider(...): Disposable

OK, I'll have a try using the create pattern.

If I understand correctly, the ImageFileProvider would provide a registerFiles?

If so, would it be preferable to have an ImageProvider, providing registerFiles, registerCheck, etc?

@feloy feloy marked this pull request as draft June 25, 2024 13:56
@feloy feloy marked this pull request as ready for review June 26, 2024 09:02
@feloy
Copy link
Contributor Author

feloy commented Jun 26, 2024

I changed the pattern, for the API to provide helper functions to the extension, to help populate the response to getFilesystemLayers
cc @benoitf

Signed-off-by: Philippe Martin <[email protected]>
Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

thanks for the big changes @feloy

I think now it's more straightforward

Copy link
Contributor

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

This looks amazing and I have no issues either with the code after going through it, amazing work!

LGTM.

@cdrage cdrage merged commit 0fb809c into containers:main Jun 27, 2024
9 checks passed
@podman-desktop-bot podman-desktop-bot added this to the 1.12.0 milestone Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants