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

Switch to 'make bin' instead of 'make binary' #302

Merged
merged 1 commit into from
Nov 30, 2016

Conversation

cdrage
Copy link
Member

@cdrage cdrage commented Nov 21, 2016

I keep mistyping this when creating the binary as per other projects
that use it commonly (it's usually make bin instead of make binary).

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 21, 2016
Copy link
Contributor

@sebgoa sebgoa left a comment

Choose a reason for hiding this comment

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

Actually, if we want to modify this we should:

  • Add the license header
  • Make make the default which creates the binary
  • change binary-cross to cross
  • Make all target also runs the validation checks...

@cdrage
Copy link
Member Author

cdrage commented Nov 24, 2016

@sebgoa do we really need to put a license header in every file? Not even Docker has that (and it's pretty much the largest Go project out there: https://github.com/docker/docker/blob/master/Makefile)

@cdrage cdrage force-pushed the switch-to-make-bin branch from 0221c14 to caab297 Compare November 24, 2016 17:01
@cdrage
Copy link
Member Author

cdrage commented Nov 24, 2016

@sebgoa
Updated the PR and to answer your questions:
make already builds binary by default
binary-cross switched to just cross
yeah... validation tests should be added, i'll add this once gofmt + go lint has been added here: https://github.com/kubernetes-incubator/kompose/pull/259/files

@kadel
Copy link
Member

kadel commented Nov 25, 2016

yeah... validation tests should be added, i'll add this once gofmt + go lint has been added here: https://github.com/kubernetes-incubator/kompose/pull/259/files

added ;-)

@cdrage cdrage force-pushed the switch-to-make-bin branch from caab297 to 7851fc1 Compare November 28, 2016 14:03
@cdrage
Copy link
Member Author

cdrage commented Nov 28, 2016

@kadel thanks :)

@sebgoa updated with all your suggestions now! Good for review 👍

@kadel
Copy link
Member

kadel commented Nov 29, 2016

@cdrage don't forget to update .travis.yml 😉

@sebgoa
Copy link
Contributor

sebgoa commented Nov 29, 2016

@cdrage and yes we need the license header. I don't care what Docker does. Any file that does not have the license header is not properly licensed.. that's how you apply an ASL license throughout your code base.

Copy link
Contributor

@sebgoa sebgoa left a comment

Choose a reason for hiding this comment

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

yup, need an updated travis file.

@@ -89,7 +89,7 @@ $ CGO_ENABLED=0 GO15VENDOREXPERIMENT=1 go build -o kompose main.go
To create a multi-platform binary, use the `binary-cross` command via `make`:

```console
$ make binary-cross
$ make bin-cross
Copy link
Contributor

Choose a reason for hiding this comment

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

the Makefile uses the cross target not the bin-cross ?

@cdrage cdrage force-pushed the switch-to-make-bin branch from 7851fc1 to 5151b2a Compare November 29, 2016 14:07
@cdrage
Copy link
Member Author

cdrage commented Nov 29, 2016

@sebgoa done. added license and updated readme.

@sebgoa
Copy link
Contributor

sebgoa commented Nov 29, 2016

well, lots of travis errors

I keep mistyping this when creating the binary as per other projects
that use it commonly (it's usually `make bin` instead of `make binary`).
@cdrage cdrage force-pushed the switch-to-make-bin branch from 5151b2a to c87e6cd Compare November 29, 2016 14:56
@cdrage
Copy link
Member Author

cdrage commented Nov 30, 2016

@sebgoa Travis passes now. Forgot to update .travis.yml

@sebgoa sebgoa merged commit d534370 into kubernetes:master Nov 30, 2016
@cdrage cdrage deleted the switch-to-make-bin branch March 30, 2017 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants