-
Notifications
You must be signed in to change notification settings - Fork 25
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
pubsys: enable the use of crane to publish kits #317
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -20,7 +20,6 @@ pipesys = { version = "0.1", path = "../pipesys" } | |||
rand = { version = "0.8", default-features = false, features = ["std", "std_rng"] } | |||
regex = "1" | |||
reqwest = { version = "0.11", default-features = false, features = ["rustls-tls", "blocking"] } | |||
semver = { version = "1", features = ["serde"]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could separate all these into a cleanup commit (but it's a good change and fine to include in this PR)
Might be good to add a cargo machete
lint to CI.
let mut oci_archive = TarArchive::new(&mut oci_file); | ||
oci_archive | ||
.unpack(temp_dir.path()) | ||
.context(error::ArchiveExtractSnafu)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How much time does this step take? It's sort of nice to have just the tar archive as the output file from rpm2kit
, but if it's a meaningful speed-up, we could have the unpacked contents be the "preferred" form and create the tar archive as a temporary file in the (already slow) Docker path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested this with just the tar
command against a recent build of the core kit. unpack took 16 seconds, and recompressing the contents took 1.5 seconds - so could definitely be an optimization worth taking. docker
would take even longer to push an image but 1.5 seconds is pretty insignificant comparatively
Signed-off-by: Sam Berning <bernings@amazon.com>
Noticed a few crates had unused dependencies so I ran `cargo-machete` and cleaned up all of the ones it reported, barring the tools that `twoliter` includes with `bindeps`. Signed-off-by: Sam Berning <bernings@amazon.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Issue number:
Closes #316
Description of changes:
Adds methods to
oci_cli_wrapper::ImageTool
for pushing individual-arch kit images and the multi-arch manifest, and implements these methods for both docker and crane.Additionally, moves the
DockerArchitecture
enum intooci-cli-wrapper
, since it is mainly used while interacting with the OCI CLI tools.Finally, cleaned up unused dependencies across Twoliter with
cargo-machete
(barring all of thebindeps
dependencies for Twoliter)Testing done:
Built twoliter, and tried:
All of the above pushed as expected, and left the system in the correct state afterwards (i.e. cleaned up tempdirs and unnecessary docker images)
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.