-
Notifications
You must be signed in to change notification settings - Fork 24
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
improve Makefile #843
base: main
Are you sure you want to change the base?
improve Makefile #843
Conversation
725573c
to
b5a6438
Compare
b5a6438
to
3a71599
Compare
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.
Several notes:
- Some parts require additional explanation
- If we're installing
jq
we need to make sure that the samejq
is used in all our scripts - Issue with
unzip
reported in the Makefile does not check for required binaries presence. #512 is not covered
e86b4ea
to
077133e
Compare
- add jq to tools to be installed with make cli-install - add check for unzip - refactor Signed-off-by: Satyam Bhardwaj <[email protected]>
077133e
to
2557f64
Compare
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.
Target specific vars look bad
Please rebase
$(FLUX_HELM_CRD): | $(EXTERNAL_CRD_DIR) | ||
FLUX_HELM_VERSION ?= $(shell go mod edit -json | $(JQ) -r '.Require[] | select(.Path == "github.com/fluxcd/helm-controller/api") | .Version') | ||
FLUX_HELM_NAME ?= helm | ||
FLUX_HELM_CRD ?= $(EXTERNAL_CRD_DIR)/$(FLUX_HELM_NAME)-$(FLUX_HELM_VERSION).yaml |
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 don't think it's justified here, since:
- Syntax looks incorrect
- You introduced duplication
- You didn't add
jq
to the target requirements (I assume that was the idea)
I suggest you just change jq
-> $(JQ)
in the global definitions and leave it as it were.
make cli-install
The Makefile is structured in a way that certain variable assignments at the top level utilize external tools like jq and yq through $() functions. These tools (jq and yq) are not installed when Make begins parsing the Makefile. Their installation is handled by the cli-install target, which occurs after the top-level variable assignments.
Fixes #512