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

enhancement(ci): Add release docker image #844

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

leojonathanoh
Copy link

@leojonathanoh leojonathanoh commented Mar 1, 2023

This adds a multiarch docker image, based on previous work in #245.

Since I'm not an approved collaborator on this repo with read access to repository secrets, the CI will not be able to login to Docker Hub and push images. To show it works, I'm building the same branch on my fork PR to demonstrate that it works. leojonathanoh#1

TODO

  • Dockerfile to build CNI plugins. Bonus points for playing nice with dependabot.
  • Multiarch Dockerfile for released "installer" image
  • Fixup release.sh to use this (or some equivalent)
  • Build releases automatically via GitHub and push them to ghcr
  • Cosign all the things! (might need validation)

Implements #806

Signed-off-by: Leonard Jonathan Oh [email protected]

name: docker
on:
push:
# TODO: Build on main?
Copy link
Member

Choose a reason for hiding this comment

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

The question here is, basically, should we build / push on merge? Right?

My first thought is: sure, why not? As long as we aren't bumping the latest tag.

Copy link
Author

@leojonathanoh leojonathanoh Mar 3, 2023

Choose a reason for hiding this comment

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

EDIT: yes, this pushes only on main and tags.

.github/workflows/docker.yml Outdated Show resolved Hide resolved
@squeed
Copy link
Member

squeed commented Mar 3, 2023

This looks awesome! Thanks so much for taking this on!

I have a few small requests, but they're just the cherry on top. If you've got time, that'd be great, but I really want to get this in.

(p.s. this needs a signed-off-by :-) )

@squeed
Copy link
Member

squeed commented Mar 3, 2023

Also, looks like the new yamllint is mad at something :-p

name: docker
on:
push:
# TODO: Build on main?
Copy link
Author

@leojonathanoh leojonathanoh Mar 3, 2023

Choose a reason for hiding this comment

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

EDIT: yes, this pushes only on main and tags.

.github/workflows/docker.yml Outdated Show resolved Hide resolved
docker-installer/install_cni_plugins.sh Outdated Show resolved Hide resolved
docker-installer/install_cni_plugins.sh Outdated Show resolved Hide resolved
.github/workflows/docker.yml Outdated Show resolved Hide resolved
@leojonathanoh leojonathanoh force-pushed the feature/add-release-docker-image branch from 76a6cf4 to 1fa8df7 Compare March 3, 2023 13:44
@leojonathanoh
Copy link
Author

Do i have to signoff all commits? Or just in the PR body?

@leojonathanoh leojonathanoh force-pushed the feature/add-release-docker-image branch from 3408e60 to 73df0e2 Compare March 3, 2023 19:27
@squeed
Copy link
Member

squeed commented Mar 6, 2023

Do i have to signoff all commits? Or just in the PR body?

All commits :-).

@squeed
Copy link
Member

squeed commented Mar 6, 2023

But we should squash this down to one or two commits before merging.


# This is the final, minimal container
FROM busybox as final
COPY docker-installer/install_cni_plugins.sh /script/install_cni_plugins.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

You may need LABEL command to specify some of label, as github document mentioned, such as org.opencontainers.image.source

As I worked similar stuff in another repo, this org.opencontainers.image.source is required.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the tip. Any recommendation on which LABELs to add?

Copy link
Author

Choose a reason for hiding this comment

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

managed to add all the org.opencontainers.image.xxx labels, hope i didn't miss anything.

@leojonathanoh
Copy link
Author

All commits :-).

But we should squash this down to one or two commits before merging.

will squash it down and sign once the PR is done :)

@leojonathanoh
Copy link
Author

oh yes, I was wondering about how to tie the release CI process with this docker image's binaries, so there is one source of binaries as discussed in #245 (comment). Can't seem to find out how the release process goes as far as looking around.

@leojonathanoh leojonathanoh force-pushed the feature/add-release-docker-image branch from 2c28635 to 9abc4f6 Compare March 23, 2023 13:26
@leojonathanoh
Copy link
Author

squashed my commits down and signed off. i'm not sure what to do with whether previous work should be signed off too?

@squeed
Copy link
Member

squeed commented May 9, 2023

@leojonathanoh can you rebase on main and squash to a single commit please? Then we can get this awesome PR merged!

@leojonathanoh
Copy link
Author

sure, i'll do this tomorrow when i manage to get some time.

@leojonathanoh leojonathanoh force-pushed the feature/add-release-docker-image branch from 9abc4f6 to 4772ad7 Compare May 11, 2023 16:27
@onedr0p
Copy link

onedr0p commented Nov 5, 2023

Any update on this PR @squeed ? Squash and merge and release 🚀

@onedr0p
Copy link

onedr0p commented Dec 8, 2023

@leojonathanoh looks like you might need to rebase and fix any conflicts since it's taken this long for a merge :/

@leojonathanoh
Copy link
Author

Sure 😄

@leojonathanoh leojonathanoh force-pushed the feature/add-release-docker-image branch from 4772ad7 to 747294f Compare December 9, 2023 10:07
Co-authored-by: Michael Wyraz <[email protected]>
Signed-off-by: Leonard Jonathan Oh <[email protected]>
@leojonathanoh
Copy link
Author

Passed on my fork, see leojonathanoh#1, where I have fork secrets that don't work on the CI here.

Before merging, will need to remove the code marked with TODO in the CI files.

@onedr0p
Copy link

onedr0p commented Jan 2, 2024

It's unfortunate that this as been pending so long, it seems like the maintainers are ignoring this or don't find value in this PR 😭

@leojonathanoh
Copy link
Author

it's fine 😄 perhaps it will get merged. Guess not many in the community need a docker image for updating plugins 😬

@MikeZappa87
Copy link
Contributor

@leojonathanoh hello, sorry things get lost in the shuffle! The best way is to join the weekly sync and put an item on the agenda! Sorry for the delay!

@micw
Copy link

micw commented Apr 21, 2024

it's fine 😄 perhaps it will get merged. Guess not many in the community need a docker image for updating plugins 😬

Hello,

I was the author of the initial request (#245). After a few years I'm back to "multus-cni" and had the hope this is meanwhile merged :-(
Just wanted you to know that this is one of the use cases where a docker image of the plugins would be really helpful.

@leojonathanoh
Copy link
Author

it's fine 😄 perhaps it will get merged. Guess not many in the community need a docker image for updating plugins 😬

Hello,

I was the author of the initial request (#245). After a few years I'm back to "multus-cni" and had the hope this is meanwhile merged :-( Just wanted you to know that this is one of the use cases where a docker image of the plugins would be really helpful.

Thanks for your previous work. Hope to see this merged too 😄

@onedr0p
Copy link

onedr0p commented Apr 21, 2024

image

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.

6 participants