Skip to content

values: refactor image block #174

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

Merged
merged 1 commit into from
Apr 11, 2025

Conversation

egabancho
Copy link
Member

Does this close #14?

@egabancho egabancho requested a review from lindhe April 1, 2025 14:13
@lindhe
Copy link
Contributor

lindhe commented Apr 4, 2025

Does this close #14?

Impossible to know, the author of that issue never described why that wanted the change. 😄 But if we do any refactoring like this, let's just mark their issue as closed and then let them reopen it if we didn't fix it.

In my opinion, replacing the image string with an `image object that includes repo, registry, image name and tag (like you do here) is good. It's also good to have a single global for the image, I think. But I would like us to keep the option of overriding the image for individual component. Makes it easier to experiment if it's possible to overwrite them.

@egabancho
Copy link
Member Author

I don't think allowing different images for the various components is such a good idea. All sorts of things can happen, and that's why everywhere else the same one is used. This is not the best example, but it's the one I have at hand https://github.com/inveniosoftware/cookiecutter-invenio-rdm/blob/master/%7B%7Bcookiecutter.project_shortname%7D%7D/docker-compose.full.yml ☺️

I wouldn't mind removing the deprecation warning from [web|worker].image and adding a "do it at your own risk" type of message. But honestly, I believe it's not worth the effort of adding an image block to web, worker, etc.

@lindhe
Copy link
Contributor

lindhe commented Apr 7, 2025

Fair enough, I won't contest that. Let's keep it a single global images object for all Invenio containers then.

Review incoming.

Copy link
Contributor

@lindhe lindhe left a comment

Choose a reason for hiding this comment

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

Pretty much LGTM. Just have a few minor comments/questions I'd like feedback on before approval.

@egabancho egabancho requested a review from lindhe April 11, 2025 11:55
@egabancho egabancho merged commit 443e208 into inveniosoftware:master Apr 11, 2025
1 check passed
@egabancho egabancho deleted the docker-image branch April 11, 2025 16:26
lindhe added a commit that referenced this pull request Apr 16, 2025
When `.Values.image.imagePullSecrets` was introduced in #174, we opted
to use the same format for `imagePullSecrets` list as used in
Kubernetes' PodSpec; it's a list of objects that requires each list
element to have the key `name`.
We failed to indicate this in the example given in the comment.
This change fixes that.
lindhe added a commit that referenced this pull request Apr 17, 2025
When `.Values.image.imagePullSecrets` was introduced in #174, we opted
to use the same format for `imagePullSecrets` list as used in
Kubernetes' PodSpec; it's a list of objects that requires each list
element to have the key `name`.
We failed to indicate this in the example given in the comment.
This change fixes that.
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.

job/cronjob parametrize image
2 participants