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.

Sign up for GitHub

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

Jump to bottom

Add support for --digestfile for skopeo sync #2402

Merged
merged 1 commit into from
Oct 8, 2024
Merged

Add support for --digestfile for skopeo sync #2402

merged 1 commit into from
Oct 8, 2024

Conversation

ankitforcode
Copy link
Contributor

@ankitforcode ankitforcode commented Aug 19, 2024 •
edited
Loading

Added --digestfile for skopeo sync option

  • This will create the digest file if not exists.
  • Will append the sha and image reference for each image that is copied separated by space.
  • This option would not be available in dry-run flag is set.

@packit-as-a-service Packit-as-a-Service
Copy link

We were not able to find or create Copr project packit/containers-skopeo-2402 specified in the config with the following error:

Cannot create a new Copr project (owner=packit project=containers-skopeo-2402 chroots=['fedora-39-aarch64', 'fedora-40-x86_64', 'fedora-39-x86_64', 'fedora-rawhide-aarch64', 'fedora-40-aarch64', 'fedora-rawhide-x86_64', 'fedora-eln-aarch64', 'fedora-eln-x86_64']): Copr: 'packit/containers-skopeo-2402' already exists. Copr HTTP response is 400 BAD REQUEST.

Unless the HTTP status code above is >= 500, please check your configuration for:

  1. typos in owner and project name (groups need to be prefixed with @)
  2. whether the project name doesn't contain not allowed characters (only letters, digits, underscores, dashes and dots must be used)
  3. whether the project itself exists (Packit creates projects only in its own namespace)
  4. whether Packit is allowed to build in your Copr project
  5. whether your Copr project/group is not private

@ankitforcode ankitforcode changed the title DO NOT MERGE feat: adding manifest output for skopeo sync Add support for --digestfile for skopeo sync Aug 19, 2024
@ankitforcode ankitforcode reopened this Aug 19, 2024
mtrmac
mtrmac requested changes Aug 19, 2024
View reviewed changes
Copy link
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Thanks!

Did you try running the PR? AFAICS this would keep overwriting the same file with digests, and in effect only record the last digest of them. I don’t see how that’s useful.

@mtrmac
Copy link
Contributor

mtrmac commented Aug 19, 2024

Also, https://github.com/containers/skopeo/blob/main/CONTRIBUTING.md#sign-your-prs ; we can’t really even look at PRs with unclear copyright status.

@ankitforcode ankitforcode force-pushed the feature/adding-manifest-output-skopeo-sync branch 2 times, most recently from 0172d87 to 7142b58 Compare August 19, 2024 15:44
@ankitforcode
Copy link
Contributor Author

Thanks!

Did you try running the PR? AFAICS this would keep overwriting the same file with digests, and in effect only record the last digest of them. I don’t see how that’s useful.

hey @mtrmac
Thanks for reviewing, I have fixed the code in a way that each sha for the image that is copied would be appended to the file once copied. This option would also not be available in case the dry-run flag is set.

@ankitforcode ankitforcode requested a review from mtrmac August 20, 2024 12:08
@ankitforcode
Copy link
Contributor Author

Thanks!

Did you try running the PR? AFAICS this would keep overwriting the same file with digests, and in effect only record the last digest of them. I don’t see how that’s useful.

Done.

@ankitforcode ankitforcode marked this pull request as draft August 27, 2024 09:21
@ankitforcode ankitforcode marked this pull request as ready for review August 27, 2024 09:22
@TomSweeneyRedHat
Copy link
Member

LGTM

@ankitforcode ankitforcode force-pushed the feature/adding-manifest-output-skopeo-sync branch from 1b861a3 to 4932405 Compare August 29, 2024 08:48
@ankitforcode
Copy link
Contributor Author

Thanks!

Did you try running the PR? AFAICS this would keep overwriting the same file with digests, and in effect only record the last digest of them. I don’t see how that’s useful.

@mtrmac can you please give it a poke.

mtrmac
mtrmac reviewed Aug 29, 2024
View reviewed changes
@mtrmac mtrmac linked an issue Aug 29, 2024 that may be closed by this pull request
skopeo sync --digestfile option #1497
Closed
@mtrmac mtrmac added the kind/feature A request for, or a PR adding, new functionality label Aug 29, 2024
@ankitforcode ankitforcode requested a review from mtrmac August 31, 2024 10:03
mtrmac
mtrmac requested changes Sep 2, 2024
View reviewed changes
@ankitforcode ankitforcode requested a review from mtrmac September 24, 2024 20:21
@ankitforcode ankitforcode force-pushed the feature/adding-manifest-output-skopeo-sync branch from 43746a1 to 279ee95 Compare September 25, 2024 08:11
@ankitforcode
Copy link
Contributor Author

Thanks!

Did you try running the PR? AFAICS this would keep overwriting the same file with digests, and in effect only record the last digest of them. I don’t see how that’s useful.

Ran the changes on local and validated the output for manifest file.

mtrmac
mtrmac reviewed Sep 27, 2024
View reviewed changes
@ankitforcode ankitforcode requested a review from mtrmac October 4, 2024 09:00
mtrmac
mtrmac reviewed Oct 7, 2024
View reviewed changes
Copy link
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Thanks!

One last thing, and please squash the commits into one.

@ankitforcode
* Added option to create digest file for syncing images.
10a9e24 
* Digest file output would have docker reference of source
and sha of of the mainfest sync'd with the target. This
file would not be created if dry-run flag is enabled
* improved the sync document to include the correct output for manifest file.
* added new line for the manifest file once all images are sync'd
* Ensuring we log on manifest digest if the copy operation was successful.
* Check for errors if any once sync process is complete.
* Ensure to capture the failure when closing the manifest file.
* Ensure we are not writing manifest sha for failed copy of imagesand aborting the process in case write to file fails

Signed-off-by: Ankit Agarwal <[email protected]>
@ankitforcode ankitforcode force-pushed the feature/adding-manifest-output-skopeo-sync branch from 88ff098 to 10a9e24 Compare October 8, 2024 12:00
mtrmac
mtrmac approved these changes Oct 8, 2024
View reviewed changes
Copy link
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Thanks!

@mtrmac mtrmac merged commit 142f20f into containers:main Oct 8, 2024
23 checks passed
@BrewTestBot BrewTestBot mentioned this pull request Nov 13, 2024
Merged
@github-actions github-actions bot mentioned this pull request Nov 17, 2024
Open
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Jan 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mtrmac mtrmac mtrmac approved these changes

Assignees
No one assigned
Labels
kind/feature A request for, or a PR adding, new functionality locked - please file new issue/PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

skopeo sync --digestfile option
3 participants
@ankitforcode @mtrmac @TomSweeneyRedHat