-
Notifications
You must be signed in to change notification settings - Fork 561
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
makefile: use a variable to choose "docker" command #260
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.
@phlogistonjohn i think we need to update this one also
docker login -u "${QUAY_IO_USERNAME}" -p "${QUAY_IO_PASSWORD}" quay.io
Makefile
Outdated
@@ -14,6 +14,8 @@ | |||
|
|||
.PHONY: all rbdplugin cephfsplugin | |||
|
|||
DOCKERCMD:=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.
DOCKERCMD:=docker | |
CONTAINER_RUNTIME_BINARY:=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.
I can make that change but I chose 'DOCKERCMD' because it matches the variable name in a similar change I made in rook. Let me know if you feel strongly about it though.
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.
Not really but if DOCKERCMD
is podman
then that does not mean much at the end ^^. That's why I proposed CONTAINER_RUNTIME_BINARY
to make it as generic as possible.
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.
Sure. :-) But docker is the defacto standard and podman is compatible with docker cli.
I wasn't trying to replace all instances of docker, just make it so that I (and others) can build without it. Since deploy.sh is not referenced from the Makefile I think we can skip that for now. |
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 like adding some flexibility, what about just adding an eval for env variables though?
So similar to what we have for like RBD_IMAGE_NAME
maybe add an variable/eval for something more generic like:
CONTAINER_RUNTIME
It might still be limited in the options; but maybe in the future it could be worth expanding to call a shell script to do different types of container builds?
Just a thought... meanwhile I would like to see L#17 changed to eval for an env variable either way though (and default to docker).
the reason why i suggested was, you have replaced for the command to push image also, log in with docker and pushing an image with different command does not feel right to me. |
This change allows the use of alternatives to or wrappers around the normal docker command for container builds. Example 1: make image-rbdplugin CONTAINER_CMD=podman Example 2: CONTAINER_CMD=podman make image-rbdplugin Signed-off-by: John Mulligan <jmulligan@redhat.com>
28f05bf
to
23027af
Compare
@leseb @Madhu-1 @j-griffith PTAL |
This change allows the use of alternatives to or wrappers around the normal docker command when running the deploy.sh script. Example: CONTAINER_CMD=podman ./deploy.sh Signed-off-by: John Mulligan <jmulligan@redhat.com>
23027af
to
33a2fb1
Compare
@leseb ping. I think I addressed your comments, but please let me know if I overlooked something. |
@phlogistonjohn looks good thanks. |
That was super fast! thanks! :-) |
makefile: use a variable to choose "docker" command
BUG 2264002: rbd: log sitestatuses and description
This change allows the use of alternatives to or wrappers around
the normal docker command for container builds.
Example: make image-rbdplugin DOCKERCMD=podman