Skip to content

Add more no suffix version tag for k8s #431

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.

Sign up for GitHub

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

Jump to bottom
Merged
daipom merged 1 commit into from
Mar 26, 2025
Merged

Add more no suffix version tag for k8s #431

daipom merged 1 commit into from
Mar 26, 2025

Conversation

@kenhys
Copy link
Contributor

@kenhys kenhys commented Mar 25, 2025

k8s image was built with architecture-specific image to build in Dockerfile FROM, so add more tags for arm64 and amd64.

@kenhys
Copy link
Contributor Author

kenhys commented Mar 25, 2025 •
edited
Loading

We want to simplify here without internal version.

https://github.com/fluent/fluentd-kubernetes-daemonset/blob/3fe5e746c296ce7806dbffa77d4e4541dcc9f537/templates/Dockerfile.erb#L8-L13

<% if is_arm64 %>
# For multiarch build on Docker hub automated build.
FROM fluent/fluentd:<%= fluentd_ver %>-debian-arm64-1.0
<% else %>
FROM fluent/fluentd:<%= fluentd_ver %>-debian-amd64-1.0
<% end %>

@kenhys kenhys requested review from Watson1978 and daipom March 25, 2025 09:34
daipom
daipom reviewed Mar 25, 2025
View reviewed changes
Copy link
Contributor

@daipom daipom left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks!
It looks great, but I have 2 concerns.

  • Is there any possiblity that we should provide the no-suffix version tags to other than arm64 and amd64 as well?
    • It would be good enough for buidling K8s images, but wouldn't it be confusing for the user?
  • It appears to have too much logic on the CI side. Shouldn't tags be managed on the Makefile side as much as possible?

For the latter, the following methods could be considered, for example.
I think we should be able to have unspecified number of tags in the Makefile.

index 1f72bdd..aa6276c 100644
--- a/.github/workflows/docker-build.yml
+++ b/.github/workflows/docker-build.yml
@@ -72,25 +72,30 @@ jobs:
             esac
             branch=$(echo $target | cut -d'/' -f1)
             tags=$(echo $target | cut -d':' -f2-)
-            tag1=$(echo $tags | cut -d',' -f1)
-            tag2=$(echo $tags | cut -d',' -f2)
-            tag3=$(echo $tags | cut -d',' -f3)
+            edited_tags=""
+            for tag in $(echo "$tags" | tr , ' '); do
+              if [ -z "$edited_tags" ]; then
+                edited_tags="${{ env.REPOSITORY }}:$tag"
+              else
+                edited_tags+=",${{ env.REPOSITORY }}:$tag"
+              fi
+            done
             case $component in
             *alpine*)
               echo "CONTEXT=${branch}/${component}" >> ${GITHUB_ENV}
-              echo "ALPINETAGS=${{ env.REPOSITORY }}:${tag1},${{ env.REPOSITORY }}:${tag2},${{ env.REPOSITORY }}:${tag3}" >> ${GITHUB_ENV}
+              echo "ALPINETAGS=$edited_tags" >> ${GITHUB_ENV}
               ;;
             *arm64*)
               echo "CONTEXT=${branch}/${component}/debian" >> ${GITHUB_ENV}
-              echo "ARM64TAGS=${{ env.REPOSITORY }}:${tag1},${{ env.REPOSITORY }}:${tag2},${{ env.REPOSITORY }}:${tag3}" >> ${GITHUB_ENV}
+              echo "ARM64TAGS=$edited_tags" >> ${GITHUB_ENV}
               ;;
             *armhf*)
               echo "CONTEXT=${branch}/${component}/debian" >> ${GITHUB_ENV}
-              echo "ARMHFTAGS=${{ env.REPOSITORY }}:${tag1},${{ env.REPOSITORY }}:${tag2},${{ env.REPOSITORY }}:${tag3}" >> ${GITHUB_ENV}
+              echo "ARMHFTAGS=$edited_tags" >> ${GITHUB_ENV}
               ;;
             *amd64*)
               echo "CONTEXT=${branch}/debian" >> ${GITHUB_ENV}
-              echo "AMD64TAGS=${{ env.REPOSITORY }}:${tag1},${{ env.REPOSITORY }}:${tag2},${{ env.REPOSITORY }}:${tag3}" >> ${GITHUB_ENV}
+              echo "AMD64TAGS=$edited_tags" >> ${GITHUB_ENV}
               ;;
             esac
           done
diff --git a/Makefile b/Makefile
index 36c1a4e..7823364 100644
--- a/Makefile
+++ b/Makefile
@@ -18,7 +18,7 @@
 IMAGE_NAME := fluent/fluentd
 X86_IMAGES := \
        v1.18/alpine:v1.18.0-1.1,v1.18-1,edge \
-       v1.18/debian:v1.18.0-debian-amd64-1.1,v1.18-debian-amd64-1,edge-debian-amd64
+       v1.18/debian:v1.18.0-debian-amd64-1.1,v1.18-debian-amd64-1,v1.18.0-debian-amd64,edge-debian-amd64
 #      <Dockerfile>:<version>,<tag1>,<tag2>,...
 
 # Define images for running on ARM platforms
@@ -27,7 +27,7 @@ ARM_IMAGES := \
 
 # Define images for running on ARM64 platforms
 ARM64_IMAGES := \
-       v1.18/arm64/debian:v1.18.0-debian-arm64-1.1,v1.18-debian-arm64-1,edge-debian-arm64 \
+       v1.18/arm64/debian:v1.18.0-debian-arm64-1.1,v1.18-debian-arm64-1,v1.18.0-debian-arm64,edge-debian-arm64 \
 
 WINDOWS_IMAGES := \
        v1.18/windows-ltsc2019:v1.18.0-windows-ltsc2019-1.1,v1.18-windows-ltsc2019-1 \

@kenhys
Add more no suffix version tag for k8s
60563c2 
k8s image was built with architecture specific image
to build in Dockerfile FROM, so add more tags for arm64 and amd64.

Signed-off-by: Kentaro Hayashi <[email protected]>
@kenhys kenhys force-pushed the more-arch-version branch from fc7de75 to 60563c2 Compare March 26, 2025 04:28
@kenhys
Copy link
Contributor Author

kenhys commented Mar 26, 2025

Thanks, that feedback is really reasonable!

@kenhys
Copy link
Contributor Author

kenhys commented Mar 26, 2025

Is there any possiblity that we should provide the no-suffix version tags to other than arm64 and amd64 as well?

There is no plan to ship armhf image yet, but for consistency, it might be better to ship it too.
Fixed it.

daipom
daipom approved these changes Mar 26, 2025
View reviewed changes
Copy link
Contributor

@daipom daipom left a comment

Choose a reason for hiding this comment

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

Thanks!

@daipom daipom merged commit 240515a into fluent:master Mar 26, 2025
6 checks passed
@kenhys kenhys deleted the more-arch-version branch March 26, 2025 04:49
kenhys added a commit to kenhys/fluentd-docker-image that referenced this pull request Mar 27, 2025
@kenhys
Backport(v1.16): Add more no suffix version tag for k8s
e3e7baa 
See fluent#431

Signed-off-by: Kentaro Hayashi <[email protected]>
@kenhys kenhys mentioned this pull request Mar 27, 2025
Merged
kenhys added a commit that referenced this pull request Mar 27, 2025
@kenhys
Backport(v1.16): Add more no suffix version tag for k8s (#432)
7511745 
See #431

Signed-off-by: Kentaro Hayashi <[email protected]>
@kenhys kenhys mentioned this pull request May 8, 2025
Merged
@daipom daipom mentioned this pull request Jul 31, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daipom daipom daipom approved these changes

@Watson1978 Watson1978 Awaiting requested review from Watson1978

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kenhys @daipom