-
Notifications
You must be signed in to change notification settings - Fork 173
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
Clean up Makefile variables and comments #298
Clean up Makefile variables and comments #298
Conversation
export GOPATH | ||
export GOBIN | ||
|
||
# Docker |
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.
even after the refactoring i would prefer if we could group the variables according to their related usage, e.g have a comment stating that the following variables are docker related so they are not bunched in a single long list of variables.
@@ -37,11 +41,9 @@ ifdef STATIC | |||
LDFLAGS=-a -ldflags '-extldflags \"-static\"' | |||
endif | |||
|
|||
# Go tools | |||
GO = go |
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.
While this is best practice in C/C++ where different compilers are used, i think we only have one compiler in go right ?
So im fine with not having this as a parameter (i also see that other parts of this makefile directly reference go compiler directly)
|
||
# Docker image | ||
# To pass proxy for Docker invoke it as 'make image HTTP_POXY=http://192.168.0.1:8080' |
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.
nit: i think its not half bad to keep this comment.
Keep all variables at the top of the file Remove unused and/or unnecessary variables Delete unnecessary comments
Signed-off-by: Rymsza, MonikaX <monikax.rymsza@intel.com>
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.
LGTM Thanks!
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.
Thanks for the changes
Keep all variables at the top of the file
Remove unused and/or unnecessary variables
Delete unnecessary comments
Fixes #291