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

pkg/oci: Do not trim repository prefix if default. #2401

Merged
merged 1 commit into from
Jan 30, 2024
Merged

Conversation

eiffel-fl
Copy link
Member

Otherwise, it impedes image removing by only untagging them.

Fixes: bd5578b ("oci: Use default registry for Inspektor Gadget")

@mqasimsarfraz
Copy link
Member

Otherwise, it impedes image removing by only untagging them.

It was our intent to have similar UX as docker. Can't we just append the prefix to image remove if needed or the issue is coming from oras-go?

@mauriciovasquezbernal
Copy link
Member

Otherwise, it impedes image removing by only untagging them.

It was our intent to have similar UX as docker. Can't we just append the prefix to image remove if needed or the issue is coming from oras-go?

I also see these two options:

  • Make listGadgetImages return the image with the full url, and trim the prefix in ListGadgetImages
  • Add a trimDefaultPrefix parameter to listGadgetImages, it'll be true when called from ListGadgetImages, but false when called from DeleteGadgetImage

@eiffel-fl
Copy link
Member Author

Otherwise, it impedes image removing by only untagging them.

It was our intent to have similar UX as docker. Can't we just append the prefix to image remove if needed or the issue is coming from oras-go?

No problem on oras-go side but this change broke image removal.
I opened this PR to discuss this.

I also see these two options:

* Make `listGadgetImages` return the image with the full url, and trim the prefix in `ListGadgetImages`

This is also my opinion, as we just want to hide the default registry to user.
I will tweak to code to export the variables used for the default registry and trim it here.

* Add a `trimDefaultPrefix` parameter to `listGadgetImages`, it'll be true when called from `ListGadgetImages`, but false when called from `DeleteGadgetImage`

@mauriciovasquezbernal
Copy link
Member

Actually my proposal was to make this on ListGadgetImages. I don't think we should expose the default registry details outside the pkg/oci package.

@eiffel-fl
Copy link
Member Author

Actually my proposal was to make this on ListGadgetImages. I don't think we should expose the default registry details outside the pkg/oci package.

OK, but in this case we need a loop here while using the extractor would avoid this.

…n CLI.

Otherwise, it impedes image removing by only untagging them.
A repository extractor was added on the command side to keep the behavior
introduced by the fixed commit.

Fixes: bd5578b ("oci: Use default registry for Inspektor Gadget")
Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
@mauriciovasquezbernal
Copy link
Member

That's why I proposed the second option:

Add a trimDefaultPrefix parameter to listGadgetImages, it'll be true when called from ListGadgetImages, but false when called from DeleteGadgetImage

@eiffel-fl
Copy link
Member Author

That's why I proposed the second option:

Add a trimDefaultPrefix parameter to listGadgetImages, it'll be true when called from ListGadgetImages, but false when called from DeleteGadgetImage

I find this even more hacky.
Can we merge it? It addresses the underlying problem and I would like to move forward with #2402.

@mauriciovasquezbernal
Copy link
Member

As I mentioned before, I think we shouldn't expose the default prefix outside of this module. It'll fix this immediate issue, but then all users of this will have to be aware of the default domain, so I prefer to keep the logic only on this module for now.

@eiffel-fl
Copy link
Member Author

Damn, I forgot to push the new version using only local variables.

Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@eiffel-fl
Copy link
Member Author

Thank you for the review!

@eiffel-fl eiffel-fl merged commit e385125 into main Jan 30, 2024
55 of 56 checks passed
@eiffel-fl eiffel-fl deleted the francis/fix-rmi branch January 30, 2024 02:38
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

3 participants