-
Notifications
You must be signed in to change notification settings - Fork 92
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
include fluentd latest base build #706
include fluentd latest base build #706
Conversation
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 LGTM overall - however I have some minor feedback.
We probably want to match the versions of the images with that of upstream.
So we would take the version we mirror from here:
And go find the equivalent tag (in this case 4.8.0) on the repo's Github, and find the default they set for the tag version: https://github.com/kube-logging/logging-operator/blob/f8081b7bdbd2622820d2384fbe86b58873dc8db2/pkg/sdk/logging/api/v1beta1/logging_types.go#L175
In this case we see it is set to v1.16-4.8-full
which is their rolling release tag, so we have to use that to resolve the static tag since this will change. And the current one seems to resolve to v1.16-4.8-full-build.149
So that's the value we should use to mirror this image.
Note - might be worth considering what other images we mirror for logging and if we are using the expected versions. Taking steps to properly mirror those might be good too. (i.e. we use rancher/mirrored-jimmidyson-configmap-reload, but should/could probably mirror the same one upstream uses ghcr.io/kube-logging/config-reloader
)
This change LGTM - it sounds like there is some work necessary to identify and pin the correct tag used by the chart though? I'll wait for that conversation to resolve before approving and merging. |
I appreciate the thorough explanation, thanks! Just updated the image tag to match the expected one. Also, I'll open a separate issue to further investigate the use of mirrored images for logging as suggested. |
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.
🚢
@brandond I might need some help with merging this when you're available :D Also, the DockerHub repo for this image seems to not have any deployed tags even though we already had one in entry for it in the image-list file. Do you know what might be the reason? |
https://github.com/rancher/image-mirror/actions/runs/10574628248/job/29296558931#step:8:3621
It looks like EIO has not granted the bot account access to publish to that repo. That should probably be fixed before we add additional tags. cc @dharmit |
Pull Request Checklist
SOURCE
is still accurate and upstream hasn't been migrated to a new regitry or repo (if they've migrated, a new repo request to EIO is needed to comply with theSOURCE DESTINATION TAG
pattern)SOURCE DESTINATION TAG
>= v2.7
).Types of Change
New image tag
Linked Issues
#1366
Additional Notes
Final Checks after the PR is merged