-
Notifications
You must be signed in to change notification settings - Fork 25
Add Makefile #54
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
Add Makefile #54
Conversation
da8b754
to
98181db
Compare
|
||
IMAGE_NAME=codeclimate/codeclimate-duplication | ||
|
||
all: image test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would I make all
when make test
is more intention-revealing and has the same result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a holdover from the one I wrote for builder, with the idea being that just typing make
would build an image & also run tests to confirm goodness. You can, of course, run make test
explicitly.
I'm fine with removing it & saying the default task should simply be building the image. I'll update builder as well if that's preferred, since whichever way we do it should be consistent.
Actually, I'll go ahead and remove it in both places: earlier Makefiles (app, toolbox) don't do this.
FYI: I'm being nit-heavy here just because I can see this pattern spreading to other engines so it's worth getting right and pinning down style IMO. |
Nits are good! I do expect whatever we do here to become the template for other engines, so it's worth sweating the details now so we're happy with it. |
|
||
all: image test | ||
|
||
image: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I want to keep this task name: as we work with more open source tools as "first party" engines, it'll become more likely those other projects will already have makefiles, so I think "image" might be too generic. I'm wondering if we should standardize on something more explicit that we can expect to use without issue everywhere. Maybe "codeclimate_engine"? Thoughts on this @pbrisbin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. It's worth questioning, but I'm not sure I share your concern:
- An existing project with an existing Makefile is unlikely to be using
image
, IMHO. While it is generic, I've never seenmake image
in the wild - The intent of this target is (to me) not specifically to "make a codeclimate engine", but to build a docker image named
IMAGE_NAME
--make image
seems very apt
For those reasons, I'd be OK going with this for now. Could be worth sleeping on it though.
98181db
to
d177ae2
Compare
d177ae2
to
9729601
Compare
👀 @codeclimate/review |
LGTM -- so sleek! |
This could be used almost without changes in most of our engines: just change the value of IMAGE_NAME.