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

feat(docker/kumactl): make entrypoint consistent #6596

Conversation

bartsmykla
Copy link
Contributor

@bartsmykla bartsmykla commented Apr 24, 2023

Make kumactl container image entrypoint consistent with kuma-cp and kuma-dp images, where names of images refer to binaries set in entrypoint.

This PR is a follow-up after the discussion in #6593 (comment)

Checklist prior to review

Changelog: feat(docker/kumactl): make entrypoint consistent with kuma-cp and kuma-dp images

Make `kumactl` container image entrypoint consistend with `kuma-cp`
and `kuma-dp` images, where names of images refer to binaries set in
entrypoint.

Signed-off-by: Bart Smykla <bartek@smykla.com>
@bartsmykla bartsmykla requested review from michaelbeaumont, a team and jakubdyszkiewicz and removed request for a team April 24, 2023 08:03
Copy link
Contributor

@michaelbeaumont michaelbeaumont left a comment

Choose a reason for hiding this comment

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

LGTM!

@bartsmykla bartsmykla enabled auto-merge (squash) April 24, 2023 08:11
@lahabana lahabana disabled auto-merge April 24, 2023 08:11
Copy link
Contributor

@lahabana lahabana left a 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 changing the behaviour which requires changing all the docs and anyone using any script for little added value on our side.
What's the motivation behind this?

@bartsmykla
Copy link
Contributor Author

I think this is changing the behaviour which requires changing all the docs and anyone using any script for little added value on our side. What's the motivation behind this?

I think motivation is consistency. To be honest I'm not convinced that it would impact that much anyone using any script, as this was broken from 2.1.0, and so far there was only one complain about it. I fixed and backported the change in the other PR. I looked in the documentation and there is only one place where we are mentioning using kumactl image, and it was removed from the latest docs (ref. https://github.com/kumahq/kuma-website/blob/f06a058a0f28d2186289cbf25ef727649cb57e0f/app/_src/installation/docker.md#L94)

Copy link
Contributor

@michaelbeaumont michaelbeaumont left a comment

Choose a reason for hiding this comment

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

Do we need to update the Helm charts? I wanna take a look

Copy link
Contributor

@michaelbeaumont michaelbeaumont left a comment

Choose a reason for hiding this comment

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

I think we might need to set command in

args:
- '/kuma/scripts/save_crds.sh'

command: ["/busybox/busybox", "sh", "-c"]

@lahabana I think for _ctl docker images or even any docker image, the user's assumption will be that the entrypoint is the binary.

Signed-off-by: Bart Smykla <bartek@smykla.com>
@bartsmykla
Copy link
Contributor Author

@michaelbeaumont I updated this hook, but I went with sh -c instead of explicit busybox as I believe it's safer for other charts which have this one as a dependency

Copy link
Contributor

@michaelbeaumont michaelbeaumont left a comment

Choose a reason for hiding this comment

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

LGTM at least

@bartsmykla
Copy link
Contributor Author

@lahabana this also makes it container structure tests more reusable as without this, we couldn't test entrypoint for kumactl image if we want ubi images to reuse the same tests as ubi images don't use busybox

@bartsmykla
Copy link
Contributor Author

We did internal poll about this, and decided to introduce this change

@bartsmykla bartsmykla merged commit 7d6b516 into kumahq:master Apr 26, 2023
4 checks passed
@bartsmykla bartsmykla deleted the feat/make-kumactl-image-entrypoint-consistend-with-other-images branch April 26, 2023 04:05
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.

None yet

3 participants