-
Notifications
You must be signed in to change notification settings - Fork 253
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
Push Images to GHCR.io #1071
Push Images to GHCR.io #1071
Conversation
Signed-off-by: Sarath Chandra Oruganti <[email protected]>
Signed-off-by: Sarath Chandra Oruganti <[email protected]>
- "cmd/fluent-watcher/fluentbit/**" | ||
- "cmd/fluent-watcher/hooks/**" | ||
- "pkg/filenotify/**" | ||
|
||
env: | ||
FB_IMG: 'kubesphere/fluent-bit:v2.2.2' | ||
FB_IMG_DEBUG: 'kubesphere/fluent-bit:v2.2.2-debug' | ||
FB_IMG: "kubesphere/fluent-bit:${{ github.ref_name }}" # kubesphere/fluent-bit:v2.2.2 |
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.
I would like to recommend using GitHub action variables here, something like vars.FB_IMG. While testing actions on contributors side, it makes hard as they don't have permissions to push to this repo and it takes 1 or 2 commits to rollback to desired changes.
This follows to other env's as well.
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.
Be careful with ref_name though as it won't be what's expected for PRs.
None of this should be done by variables like this really, we should use the docker/metadata-action for this as it properly supports multiple tags and configuring them (e.g. for default branch, tags, etc.).
FB_IMG: "kubesphere/fluent-bit:${{ github.ref_name }}" # kubesphere/fluent-bit:v2.2.2 | ||
FB_IMG_DEBUG: "kubesphere/fluent-bit:${{ github.ref_name }}-debug" # kubesphere/fluent-bit:v2.2.2-debug | ||
|
||
FB_IMG_GHCR: "${{ github.repository }}/fluent-bit:${{ github.ref_name }}" # fluent/fluent-operator/fluent-bit:v2.2.2 |
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.
We have fluent-bit image built by other https://github.com/fluent/fluent-bit/ to differentiate between the images built by both, I am pushing the image to current repository only.
ARCH: '-arm64' | ||
FD_IMG_BASE: 'kubesphere/fluentd:v1.15.3-arm64-base' | ||
|
||
FD_IMG: "kubesphere/fluentd:${{ github.ref_name }}" # kubesphere/fluentd:v2.2.2 |
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.
Taking branch name / git tag as the image tag as I couldn't figure out what to keep.
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.
Let's switch to the metadata action for docker
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.
This looks ok but a few things I think need changing and you're also missing auth for skopeo.
We should not be using env vars to name the images, we should be using the dedicated action for this as it is much better and extensible plus will not need changing every time.
https://github.com/docker/metadata-action
e.g. https://github.com/fluent/fluent-bit/blob/9d9ac68a2b45a4cedeafbf7c5aba513f494eced6/.github/workflows/call-build-images.yaml#L107
We should be pushing to ghcr.io initially as that simplifies authentication and is consistent across the Fluent repositories.
We can just login with GITHUB_TOKEN then rather than needing a secret and it will work for forks, etc.
permissions:
contents: read
packages: write
I would then split the release to other registries out to a separate workflow, only run for tagged releases.
- "cmd/fluent-watcher/fluentbit/**" | ||
- "cmd/fluent-watcher/hooks/**" | ||
- "pkg/filenotify/**" | ||
|
||
env: | ||
FB_IMG: 'kubesphere/fluent-bit:v2.2.2' | ||
FB_IMG_DEBUG: 'kubesphere/fluent-bit:v2.2.2-debug' | ||
FB_IMG: "kubesphere/fluent-bit:${{ github.ref_name }}" # kubesphere/fluent-bit:v2.2.2 |
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.
Be careful with ref_name though as it won't be what's expected for PRs.
None of this should be done by variables like this really, we should use the docker/metadata-action for this as it properly supports multiple tags and configuring them (e.g. for default branch, tags, etc.).
|
||
jobs: | ||
build: | ||
runs-on: ubuntu-latest | ||
timeout-minutes: 30 | ||
name: Build Image for Fluent Bit | ||
outputs: |
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.
Outputs should come from steps really, e.g. the metadata action would give you a way to get this. That way if we modify something (or for PRs, non-main branch, etc.) it will carry on working.
file: ./cmd/fluent-watcher/fluentbit/Dockerfile | ||
push: true | ||
platforms: linux/amd64,linux/arm64 |
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.
This will be pretty slow btw, it may not be worthwhile to do ARM builds for PRs.
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.
We are currently building in our existing approach, so I have added.
https://github.com/fluent/fluent-operator/blob/master/Makefile#L99-L103
|
||
- name: Login to Docker Hub | ||
uses: docker/login-action@v3 | ||
with: | ||
registry: docker.io |
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.
Are we pushing to docker.io now instead? I would keep things in ghcr.io if you can particularly for PRs otherwise we have to start dealing with rate limiting.
registry_password: ${{ secrets.REGISTRY_PASSWORD }} | ||
|
||
release-image-to-gchr: |
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.
I think this is the wrong way round, all images should start in ghcr.io as it'll be public and not rate limited. We then push to the others only release images (not all the PR images or whatever too).
- name: Promote container images from ${{ inputs.source_registry }} to ${{ inputs.target_registry }} | ||
run: | | ||
echo "Promoting $SOURCE_IMAGE to $RELEASE_IMAGE" | ||
docker run --rm \ |
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.
Note we may have to drop signatures if a registry does not support it so it might be useful to make that an optional parameter?
needs: check-image-exists | ||
runs-on: ubuntu-latest | ||
steps: | ||
- name: Promote container images from ${{ inputs.source_registry }} to ${{ inputs.target_registry }} |
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.
You need to log into the registry first and then pass the auth.json to skopeo in the next step as a bind mount.
- name: Checkout code | ||
uses: actions/checkout@v4 | ||
with: | ||
fetch-depth: 0 |
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.
What's the reason for this?
with: | ||
fetch-depth: 0 | ||
|
||
- name: Login to Docker Hub |
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.
Use the input variable
run: | | ||
echo "Platform: ${{ matrix.platform }}" | ||
|
||
- name: Dockle - multi-arch |
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.
We need to support an option to ignore any false positives here as well.
PR closed? @sarathchandra24 |
I was making some changes and I got errors which to push images my personal repo; So I deleted the repo and forked it again. It closed this PR. |
Looks like this cannot be reopened, you'll submit another PR for this with all the comments resolved? @sarathchandra24 |
created a new PR: #1079 |
What this PR does / why we need it:
As specified in #1068, we would like to push current images to multiple registries, this PR starts pushing images to GitHub Container Registry ghcr.io
Which issue(s) this PR fixes:
Fixes #1068
Does this PR introduced a user-facing change?
Additional documentation, usage docs, etc.: