Fix Makefile to take vaules from ENV VAR values

[ArangoGutierrez]

make image and make push fails when trying to set a diff value to
IMAGE_BUILD_CMD, runing the default value

Signed-off-by: Carlos Eduardo Arango Gutierrez carangog@redhat.com

[ArangoGutierrez]

[eduardo@fedora-32-ws node-feature-discovery]$ IMAGE_BUILD_CMD="podman build" make image 
nfd-worker-daemonset.yaml: namespace: kube-system
nfd-worker-daemonset.yaml: image: k8s.gcr.io/nfd/node-feature-discovery:v0.6.0-32-g900c20e
nfd-daemonset-combined.yaml: namespace: kube-system
nfd-daemonset-combined.yaml: image: k8s.gcr.io/nfd/node-feature-discovery:v0.6.0-32-g900c20e
nfd-worker-job.yaml: namespace: kube-system
nfd-worker-job.yaml: image: k8s.gcr.io/nfd/node-feature-discovery:v0.6.0-32-g900c20e
nfd-master.yaml: namespace: kube-system
nfd-master.yaml: image: k8s.gcr.io/nfd/node-feature-discovery:v0.6.0-32-g900c20e
docker build --build-arg VERSION=v0.6.0-32-g900c20e \
        --build-arg HOSTMOUNT_PREFIX=/host- \
        -t k8s.gcr.io/nfd/node-feature-discovery:v0.6.0-32-g900c20e \
         ./
make: docker: Command not found
make: *** [Makefile:47: image] Error 127

[ArangoGutierrez]

/assign @marquiz

[ArangoGutierrez]

/assign @marquiz

[marquiz]

With the current Makefile you need to use overrides, like:

make image IMAGE_BUILD_CMD="podman build"

So I would say this PR rather changes the handling of variables than fixes something that is broken.

Nevertheless, this might be a rational change, making customized build possibly a bit less surprsing for users. But, in that case we should change the handling of all variables mentioned in the README in a similar fashion. Also, the commit message subject must be changed to something like

README: blah blah blah

<Explain how variable handling changes>

[ArangoGutierrez]

With the current Makefile you need to use overrides, like:

make image IMAGE_BUILD_CMD="podman build"

So I would say this PR rather changes the handling of variables than fixes something that is broken.

Nevertheless, this might be a rational change, making customized build possibly a bit less surprsing for users. But, in that case we should change the handling of all variables mentioned in the README in a similar fashion. Also, the commit message subject must be changed to something like

README: blah blah blah

<Explain how variable handling changes>

Thanks for the review, I applied the same logic to all the variables that can be defined pre-running the make command, and added some documentation on the README

[marquiz]

IMAGE_TAG_NAME should also be initialized with =?

[marquiz]

Two small comments.

In addition, please change the commit message to something I outlined in the previous review. I.e. change the formatting a bit and change the message to correspond the reality 😄

[ArangoGutierrez]

Two small comments.

In addition, please change the commit message to something I outlined in the previous review. I.e. change the formatting a bit and change the message to correspond the reality smile

Will do thanks. yeah you are right, some times I just get used to small quick messages, but good strong messages are good for maintenance in the long run

[ArangoGutierrez]

@marquiz check commit 6cdf681 , it details what is happening on this patch

[marquiz]

Will do thanks. yeah you are right, some times I just get used to small quick messages, but good strong messages are good for maintenance in the long run

Thanks, the mismatch with the reality of the previous version was quite strong 😉 The commit messages do not always to be super long, even though too long commit messages are rarely a problem.

There's still one unaddressed comment

[ArangoGutierrez]

Will do thanks. yeah you are right, some times I just get used to small quick messages, but good strong messages are good for maintenance in the long run

Thanks, the mismatch with the reality of the previous version was quite strong wink The commit messages do not always to be super long, even though too long commit messages are rarely a problem.

There's still one unaddressed comment

done

[marquiz]

Thanks @ArangoGutierrez !
/lgtm

[marquiz]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ArangoGutierrez, marquiz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [marquiz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment