build: Makefile

[paulfantom]

Can I interest you in a Makefile? 😄

This is just something on top of which you could start building.

/CC: @sallyom @wking

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: paulfantom
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: abhinavdahiya

If they are not already assigned, you can assign the PR to them by writing /assign @abhinavdahiya in a comment when ready.

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[rajatchopra]

FWIW, I created my own local Makefile to allow me to feed my habit of typing make for a build - so there is a set of people who will find it useful.
And the code here lgtm.

[crawford]

The help target is clever. I don't see the need for yet another way to trigger the build though. I'd rather we just stick to scripts since Go and Make really do not get along.

[paulfantom]

That's why I just wrapped scripts into one Makefile and not used any go <sth> command. It just gives you one, unified way to execute those scripts.

[wking]

I'm fine without a Makefile, but I agree that folks will generally expect something like this, and I don't expect it to be too hard to maintain, so I'm ok going ahead with this PR too.

[wking]

I'd rather this be a .PHONY bin/terraform, so the target matches the output, with .PHONY because we aren't declaring the upstream dependencies.

.PHONY: bin/terraform
bin/terraform:
  hack/get-terraform.sh

Pretty much everything in here is .PHONY. I'm fine if you want to have a single entry at the end too:

.PHONY: \
  bin/openshift-install \
  bin/terraform \
  help \
  etc \
  etc

[paulfantom]

For some stupid reason I had .PHONY everywhere and deleted it.

[wking]

Same as for the Terraform target, I'd rather this matched the name of the file being created. In this case, it's bin/openshift-install. And once you match, you'll need a .PHONY entry, because we aren't telling make about the Go dependencies.

[wking]

This doesn't mass my personal "utility outweighs complexity" threshold, although other maintainers may feel differently. Can we just do something like this?

[paulfantom]

I kinda like it. By writing one thing you automatically get two:

• code comments
• runtime help

So no need to maintain a part of Makefile responsible for help target.

[wking]

By writing one thing you automatically get two:

• code comments
• runtime help

Runtime help is useful for users who don't read Makefile. I don't think the comment role is important though, especially with a Makefile as short and simple as this.

A hard-coded approach to help is also harder to get wrong (e.g. this, I think?). And you'll have to expand your regexp to allow slashes for bin/terraform.

I also don't know how portable your ANSI color escapes are (e.g. does Windows' stock terminal support them?). I'd rather drop the terminal escapes.

But if you're really committed, I'll /lgtm this approach ;).

[wking]

This should be over ./pkg/.... Otherwise this looks good; why comment it out?

[crawford]

Can you also format your commit message properly? Take a look at the git history to see the general format we follow. I would use build: as the scope for this one.

[paulfantom]

Fixed.

[frobware]

The help target is clever. I don't see the need for yet another way to trigger the build though. I'd rather we just stick to scripts since Go and Make really do not get along.

From various places I find it easier to do make .... I don't normally care how that is make target is implemented; the Makefile and target rule provides the abstraction should you want to change it in the future.

[paulfantom]

Fixed.

[paulfantom]

Fixed 😄

[abhinavdahiya]

Personally, keeping the Makefile in the root makes it seem like the official way to run/build things in installer repo. And i think we have discussed it before that the hack/* script are the only supported way.
So people are free to create tooling around the scripts ... eg: switch / add TAGS,MODE for hack/build.sh etc. But a rooted Makefile doesn't seem like the way forward.

[crawford]

@abhinavdahiya and I talked it over in person and we don't think the Makefile is the right way forward. Thank you anyway for your effort.

/close

[openshift-ci-robot]

@crawford: Closing this PR.

Details

In response to this:

@abhinavdahiya and I talked it over in person and we don't think the Makefile is the right way forward. Thank you anyway for your effort.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.