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

makefile: use a variable to choose "docker" command #260

Merged
merged 2 commits into from
Mar 27, 2019

Conversation

phlogistonjohn
Copy link
Contributor

This change allows the use of alternatives to or wrappers around
the normal docker command for container builds.
Example: make image-rbdplugin DOCKERCMD=podman

Copy link
Collaborator

@Madhu-1 Madhu-1 left a 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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
DOCKERCMD:=docker
CONTAINER_RUNTIME_BINARY:=docker

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

@phlogistonjohn
Copy link
Contributor Author

phlogistonjohn commented Mar 18, 2019

@phlogistonjohn i think we need to update this one also
docker login -u "${QUAY_IO_USERNAME}" -p "${QUAY_IO_PASSWORD}" quay.io

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.

Copy link

@j-griffith j-griffith left a 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).

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Mar 19, 2019

@phlogistonjohn i think we need to update this one also
docker login -u "${QUAY_IO_USERNAME}" -p "${QUAY_IO_PASSWORD}" quay.io

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.

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>
@phlogistonjohn
Copy link
Contributor Author

  • Changed variable name to CONTAINER_CMD
  • Used ?= instead of := to support sourcing var from environment
  • Added similar variable to deploy.sh script

@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>
@phlogistonjohn
Copy link
Contributor Author

@leseb ping. I think I addressed your comments, but please let me know if I overlooked something.

@leseb
Copy link
Member

leseb commented Mar 27, 2019

@phlogistonjohn looks good thanks.

@phlogistonjohn
Copy link
Contributor Author

That was super fast! thanks! :-)

@rootfs rootfs merged commit da52e8c into ceph:csi-v1.0 Mar 27, 2019
wilmardo pushed a commit to wilmardo/ceph-csi that referenced this pull request Jul 29, 2019
makefile: use a variable to choose "docker" command
@phlogistonjohn phlogistonjohn deleted the jjm-dockercmd branch April 23, 2020 15:32
nixpanic pushed a commit to nixpanic/ceph-csi that referenced this pull request Mar 12, 2024
BUG 2264002: rbd: log sitestatuses and description
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.

5 participants