-
Notifications
You must be signed in to change notification settings - Fork 857
Add --sign-by-sq-fingerprint #2645
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
base: main
Are you sure you want to change the base?
Conversation
contrib/cirrus/runner.sh
Outdated
@@ -96,6 +96,18 @@ _run_vendor() { | |||
} | |||
|
|||
_run_build() { | |||
if [[ "$BUILDTAGS" =~ containers_image_sequoia ]]; then |
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.
@Luap99 This is broadly the kind of thing I’m thinking of doing in CI, also in Buildah/Podman to have a smoke-test.
Until we have a better packaging of the Rust code, I expect we would not opt into the build tag by default.
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.
Uh, I really really dislike this. Runtime installs like this are going to flake and break CI eventually. Rust compile times are slow so the CI times get longer.
Sure c/image and skopeo don't have much activity so maybe it can be accepted here but for podman buildah what would be the point? Just build testing seems pointless if we know it builds in c/image?
I feel like without a packaging story there is no point to ship this at all right now and try to rush this out the door. So this is hard hard blocker for me personally.
IMO the order must be find a proper place for the c -> rust library bindings, then release that as c library, then package it into fedora and then we can install it in our upstream VM images and link against it in the builds.
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.
for podman buildah what would be the point?
I intend to add a smoke-test for a new CLI option (skipped if Sequoia is not enabled). I think that can happen in any case.
And yes, then the discussion whether to enable Sequoia in CI at all, whether to enable Sequoia in releases by default, whether to test only Sequoia in CI, can happen, and I am open to any of the outcomes.
I agree it makes no sense to ship Sequoia enabled upstream by default until the library is easy to install.
Enabled in RPMs? Enabled in CI? Well, maybe?
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.
02:47 build
for the normal Skopeo Test while
11:57 build
For Skopeo test w/ Sequoia
To me that alone is not acceptable for any merge in any CI anywhere. We can't just make the dev experience an all repos so much worse just because there is one small lib that is not even in use anywhere as it is not packaged.
You can add the code but compiling this in CI on every PR is just plain wasted time/resources. To me the packaging step must happen first for the library. Then we add it into the CI images and then we can link it in build just like the other c libs without this huge overhead.
Whenever the fedora skopeo/buildah/podman rpm builds use libgpgme or Sequoia that is not a discussion I care about.
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.
Here, I think the more relevant comparison is 12:41 vs. ~22:… for the whole task; the relative difference is not as important.
Anyway, I hear you.
We always have the option of doing the build once in automation_images instead of in primary project repos like this PR is now doing. Given the expected RPM packaging, I don’t think that’s worth the effort right now, but it’s an option. Or I can test things locally and submit ~ineffective tests upstream, to be enabled later.
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.
Now updated to rely on a RPM installed in containers/automation_images#411 .
(Still blocked on both the automation_images PR, and c/image implementation, being merged.)
d1e0d83
to
507e44c
Compare
Ephemeral COPR build failed. @containers/packit-build please check. |
2 similar comments
Ephemeral COPR build failed. @containers/packit-build please check. |
Ephemeral COPR build failed. @containers/packit-build please check. |
0700cfb
to
954d692
Compare
…oia-PGP This API is proven end-to-end in containers#2876 and containers/skopeo#2645 , but it is not yet convenient to use becahse the Rust dependency has to be compiled manually. So, for now, add the API as a stub only; that allows building the CLIs and tests on top, and they will light up once the backend is added. Signed-off-by: Miloslav Trmač <[email protected]>
…oia-PGP This API is proven end-to-end in containers#2876 and containers/skopeo#2645 , but it is not yet convenient to use becahse the Rust dependency has to be compiled manually. So, for now, add the API as a stub only; that allows building the CLIs and tests on top, and they will light up once the backend is added. Signed-off-by: Miloslav Trmač <[email protected]>
817605f
to
f0d6d0c
Compare
Tests failed. @containers/packit-build please check. |
852f38b
to
b792085
Compare
@mtrmac as we have created a Skopeo 1.20 branch to be used for RHEL 9.7/10.1, I'm fine with moving forward with this now if you wish. |
6355fbc
to
c1964ba
Compare
Ephemeral COPR build failed. @containers/packit-build please check. |
b5a5c9c
to
f3dedea
Compare
e57169b
to
b3113cc
Compare
Ready for review, PTAL. |
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
NewEphemeralSigningMechanism() may, with Sequoia, return a mechanism which !SupportsSigning(); so, to determine that, test with a non-ephemeral mechanism instead. (That's likely actually faster, because we create a GNUPGHOME in these tests anyway, so we avoid creating an deleting a separate temporary directory.) Signed-off-by: Miloslav Trmač <[email protected]>
Relies on containers/automation_images#411 . Signed-off-by: Miloslav Trmač <[email protected]>
Signed-off-by: Miloslav Trmač <[email protected]>
This is containers/image#2876, integrating with the build system, and it will include new CLI and a smoke test.
Do not merge: Depends on #2681 and containers/image#2876 in c/image.