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

feat(cmd/cp): add oci-layout-path flag #1507

Conversation

mauriciovasquezbernal
Copy link
Contributor

@mauriciovasquezbernal mauriciovasquezbernal commented Sep 20, 2024

What this PR does / why we need it:

The oci-layout can't be used when the image reference contains a slash ( see issue 1505). This PR introduces a new oci-layout-path that explicitly receives the path of the oci layout fixing the parsing ambiguity.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1505

Please check the following list:

  • Does the affected code have corresponding tests, e.g. unit test, E2E test?
  • Does this change require a documentation update?
  • Does this introduce breaking changes that would require an announcement or bumping the major version?
  • Do all new files have an appropriate license header?

Copy link
Contributor

@sabre1041 sabre1041 left a comment

Choose a reason for hiding this comment

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

LGTM

cmd/oras/internal/option/target_test.go Outdated Show resolved Hide resolved
cmd/oras/internal/option/target.go Outdated Show resolved Hide resolved
@mauriciovasquezbernal mauriciovasquezbernal force-pushed the mauricio/fix-oras-cp-full-ref branch from 2ebab51 to 939d9bd Compare October 16, 2024 16:13
@mauriciovasquezbernal mauriciovasquezbernal changed the title cmd/cp: Fix --from-oci-layout when using full reference cmd/cp: Add oci-layout-path flag Oct 16, 2024
@mauriciovasquezbernal
Copy link
Contributor Author

I updated the PR with the --from-oci-layout-path idea discussed on #1505. I'll update the documentation once we agree on this approach.

Copy link

codecov bot commented Oct 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.86%. Comparing base (b6ac8e9) to head (642fcae).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1507      +/-   ##
==========================================
+ Coverage   83.84%   83.86%   +0.02%     
==========================================
  Files         118      118              
  Lines        5156     5164       +8     
==========================================
+ Hits         4323     4331       +8     
  Misses        592      592              
  Partials      241      241              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shizhMSFT shizhMSFT changed the title cmd/cp: Add oci-layout-path flag feat(cmd/cp): add oci-layout-path flag Oct 22, 2024
cmd/oras/internal/option/target.go Outdated Show resolved Hide resolved
@mauriciovasquezbernal mauriciovasquezbernal force-pushed the mauricio/fix-oras-cp-full-ref branch from 939d9bd to 91aeda1 Compare October 28, 2024 13:43
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

@shizhMSFT shizhMSFT requested a review from TerryHowe October 29, 2024 02:06
@TerryHowe
Copy link
Member

It seems like it would be easy to add a e2e test for this.

@mauriciovasquezbernal
Copy link
Contributor Author

It seems like it would be easy to add a e2e test for this.

I added some tests. There is some duplication with the existing layout tests but I didn't find an easy way to avoid it.

Copy link
Member

@TerryHowe TerryHowe 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!

Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

The oci-layout can't be used when the image
reference contains a slash ( see issue 1505). This
PR introduces a new oci-layout-path that
explicitly receives the path of the oci layout
fixing the parsing ambiguity.

The usage of this flag is:

oras cp --from-oci-layout-path <layout_path> <src> <dst>

Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>
Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>
Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>
@shizhMSFT shizhMSFT force-pushed the mauricio/fix-oras-cp-full-ref branch from 5cca9a7 to 9718f09 Compare October 31, 2024 12:31
@TerryHowe TerryHowe merged commit 9a83394 into oras-project:main Nov 3, 2024
8 checks passed
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.

oras cp fails if org.opencontainers.image.ref.name contains a full reference
4 participants