-
Notifications
You must be signed in to change notification settings - Fork 90
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
ci: added publishing intel trust authority AS docker #410
ci: added publishing intel trust authority AS docker #410
Conversation
pawelpros
commented
Jun 12, 2024
•
edited
Loading
edited
- Refactored directory structure for building KBS docker images
- Added publishing KBS intel trust authority AS docker image on ghcr.io
90c27ee
to
264c468
Compare
DCO check complains because the author does not match the sign-off. Basically it should match |
You should probably update the release script to make sure your staged image gets copied over as a release image. |
3c9266d
to
67d375b
Compare
Thanks for the tip - should be ok right now - I have also aligned with recent changes |
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.
Looks fine. I don't see anything bumping OpenSSL
Also note that we use the same naming convention for Dockerfiles in the AS dir. Do we care about this being consistent?
722eb94
to
cc0e118
Compare
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 change 1.
Why are we re-structuring the directories? What's the clear benefit of this new method over the older one?
I'd mostly be interested on whether you hit some issue with the current status of things, or not.
This kind of changes, unless those are fully triggered from the Makefile, deserve a release notes entry, as it changes files location that packagers may rely on.
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.
Change 2 is good, but I'd recommend doing it as part of its own commit(s).
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, about the change 3, which also deserves to be part of its own commit (still on this very same PR).
We're adding the possibility to build the ITA image, but ... correct me if I'm mistaken ... I think we're doing multi-arch build for all the images. With this in mind, an obvious question is ... is ITA supposed to support multi-arch? Or is it x86_64 specific?
Depending on the answer, we may have to limit this to only be built with for x86_64.
Signed-off-by: Pawel Proskurnicki <[email protected]>
cc0e118
to
3b7d11b
Compare
Signed-off-by: Pawel Proskurnicki <[email protected]>
3b7d11b
to
bc8bc27
Compare
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, thanks @pawelpros!
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