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.

Sign up for GitHub

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

Jump to bottom

Remove entrypoints in makefile. #82

Merged
merged 1 commit into from
Feb 10, 2018
Merged

Remove entrypoints in makefile. #82

merged 1 commit into from
Feb 10, 2018

Conversation

cyriltovena
Copy link
Collaborator

Removed all entrypoints in the makefile.

I've tested all changed targets except the gcloud cluster creation as I don't have a gcloud account (poor me).

I've left my proxy args for build, but I can remove it if you want though it will be transparent for you.

Resolves #28

@markmandel
Copy link
Member

This looks cool!

So I have 2 thoughts:

  1. If we go down this path - where we have $(ARGS) (which I think are all shell commands - this should be renamed to `$(SHELL_ARGS) to be consistent.

  2. I'm wondering if we add something like what is described here:
    https://stackoverflow.com/questions/8346118/check-if-a-makefile-exists-before-including-it

Basically have a directory where you can drop your user specific includes that don't get committed to git.
Basically like a /etc/foo.d/ directory in Linux.

Maybe -include ./includes.d/*.mk - and then we can drop your specific definition of DOCKER_BUILD_ARGS and you can move it into the includes.d directory just for you.

I'm 96% that should work - but what do you think of the idea? It should allow for nice flexibility without messing up things across the board for everyone.

@cyriltovena
Copy link
Collaborator Author

ok but we keep the $(DOCKER_BUILD_ARGS) for each docker build stage ? I could also use an env var ?

@markmandel
Copy link
Member

Yeah, let's definitely do that, for sure!

Maybe the includes.d dir is overkill? Just figure I didn't want to force someone to use dotenv or similar - but maybe that is actually a simpler option.

@markmandel
Copy link
Member

Also, I feel like there should be a better name than includes.d - prefs ? user-includes ?

@cyriltovena
Copy link
Collaborator Author

cyriltovena commented Feb 9, 2018 •
edited
Loading

tested your solution and it works very well. Should I rename the folder ?

markmandel
markmandel reviewed Feb 9, 2018
View reviewed changes
.gitignore Outdated
tmp
includes.d/
Copy link
Member

Choose a reason for hiding this comment

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

So I'm thinking this should be
includes.d/*

Then we can drop a nice README.md in there saying "Directory for your personal includes that won't get added to the git repository" (or find better language)

Copy link
Member

Choose a reason for hiding this comment

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

heh, would also need a !/includes.d/README.md

Otherwise the README would get ignored 😊

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm looking at this it's not working

Copy link
Member

@markmandel markmandel Feb 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Works for .gitignore - is the path correct?

(Maybe drop a .gitignore into the local-includes dir, and do it from there, as easier?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

build/local-includes/*
!build/local-includes/README.md

that works

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good with this or should I drop a new ignore file ?

Copy link
Member

Choose a reason for hiding this comment

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

LGTM!

build/Makefile Outdated
@@ -81,6 +81,9 @@ endif

include ./includes/$(osinclude)

# personal includes, excluded from the git repository
-include ./includes.d/*.mk
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if I'm sold on "includes.d" as a directory name.

user-includes ? I'm not sure. Any ideas?

Copy link
Collaborator Author

@cyriltovena cyriltovena Feb 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

well .d was at first to deferentiate directory from other files when names were the same.

https://unix.stackexchange.com/questions/4029/what-does-the-d-stand-for-in-directory-names

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not helping you :)

Copy link
Member

Choose a reason for hiding this comment

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

Hah! Yeah, I looked at the same thing - which made me think the .d wasn't a good name.

includes-local , local-includes ?

I'm leaning towards local as a name, WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

local-includes looks nice

Copy link
Member

Choose a reason for hiding this comment

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

SGTM. local-includes it is.

@cyriltovena
Copy link
Collaborator Author

wait a sec forget to rename SHELL_ARGS

@cyriltovena
Copy link
Collaborator Author

cyriltovena commented Feb 9, 2018 •
edited
Loading

but it's not consistent you used ARGS for docker also:

https://github.com/googleprivate/agones/blob/6c3df8668864532df1b2f7d079da9fa1a34f298d/build/Makefile#L115

Why do you need that one ? EDIT: for minikube-install found it

@cyriltovena
Copy link
Collaborator Author

cyriltovena commented Feb 9, 2018 •
edited
Loading

I choosed to rename to DOCKER_RUN_ARGS instead because that's what it is and we could propagate if required to other target using docker run. (for example I will need proxy for gcloud-init)

@markmandel
Copy link
Member

This looks good, just please rebase down to a single commit, and I'll approve.
(I need to add PR guidelines to CONTRIBUTING.md)

@markmandel markmandel added this to the 0.1 milestone Feb 9, 2018
@markmandel markmandel added the area/build-tools Development tooling. I.e. pretty much everything in the `build` directory. label Feb 9, 2018
@markmandel
Copy link
Member

@Kuqd Did you want to rebase this down to a single commit?

I can always try out the Github "squash and merge" button - I've not tried it before 🤷‍♂️

@cyriltovena
Copy link
Collaborator Author

cyriltovena commented Feb 10, 2018 •
edited
Loading

yes sorry I was busy, I just did it, I'm using this button a lot on our internal gitlab ! Though the merge just created another commit, so I have to squash again.

EDIT: you should be good to go !

@cyriltovena
Copy link
Collaborator Author

cyriltovena commented Feb 10, 2018 •
edited
Loading

@markmandel I don't have access to your CI to see what's wrong ! just by rebasing ?

@markmandel
Copy link
Member

@Kuqd looks like cloud builder fell over for some reason. Fired off a new build.

At some point in the future, I need to implement pushing of results to PRs. (One thing at a time)

markmandel
markmandel approved these changes Feb 10, 2018
View reviewed changes
Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

LGTM!

@markmandel markmandel merged commit f2bec0b into master Feb 10, 2018
@markmandel markmandel deleted the cleanup/remove-entrypoint branch February 10, 2018 18:42
@markmandel markmandel mentioned this pull request May 22, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markmandel markmandel markmandel approved these changes

Assignees
No one assigned
Labels
area/build-tools Development tooling. I.e. pretty much everything in the `build` directory.
Projects
None yet
Milestone
0.1
Development

Successfully merging this pull request may close these issues.
2 participants
@cyriltovena @markmandel