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

Improve generate-* targets in Makefile #1061

Merged
merged 1 commit into from
Jun 24, 2019
Merged

Improve generate-* targets in Makefile #1061

merged 1 commit into from
Jun 24, 2019

Conversation

vincepri
Copy link
Member

@vincepri vincepri commented Jun 24, 2019 •
edited
Loading

Signed-off-by: Vince Prignano [email protected]

What this PR does / why we need it:
This PR attempts to cleanup the Makefile targets specifically around the generation. It reduces build time and the amount of times we call dep-ensure and gazelle.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Related to #945

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 24, 2019
@k8s-ci-robot k8s-ci-robot requested review from detiber and justinsb June 24, 2019 15:58
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 24, 2019
@vincepri
Copy link
Member Author

/assign @ncdc

@vincepri vincepri force-pushed the simplify-improve-makefile branch from c6254c8 to f2a25ba Compare June 24, 2019 16:03
ncdc
ncdc reviewed Jun 24, 2019
View reviewed changes
Makefile Outdated
@@ -91,12 +81,18 @@ lint-full: dep-ensure ## Run slower linters to detect possible issues
bazel run //:lint-full $(BAZEL_ARGS)

.PHONY: generate
generate: clientset dep-ensure ## Generate code
go generate ./pkg/... ./cmd/...
generate: dep-ensure ## Generate code
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a dependency on dep-ensure here?

hack/verify_clientset.sh Outdated
@@ -35,7 +35,7 @@ cleanup
mkdir -p "${TMP_DIFFROOT}"
cp -a "${DIFFROOT}"/* "${TMP_DIFFROOT}"

make clientset
make generate
Copy link
Contributor

Choose a reason for hiding this comment

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

generate-clientset instead?

@ncdc
Copy link
Contributor

ncdc commented Jun 24, 2019

I didn't put this on individual lines, but in general, to me, make generate should be something you consciously decide to do, and it's unlikely it will happen frequently. How would you feel about removing generate as a dependency from targets such as manager, clusterctl, and so on?

@vincepri vincepri force-pushed the simplify-improve-makefile branch from f2a25ba to d897174 Compare June 24, 2019 16:10
@vincepri
Copy link
Member Author

@ncdc Done

@vincepri vincepri force-pushed the simplify-improve-makefile branch 2 times, most recently from 33761cd to 471dd74 Compare June 24, 2019 16:26
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 24, 2019
@vincepri vincepri force-pushed the simplify-improve-makefile branch from 471dd74 to 23fe075 Compare June 24, 2019 16:27
@vincepri vincepri force-pushed the simplify-improve-makefile branch from 23fe075 to 5e9e7ea Compare June 24, 2019 16:41
@vincepri
Copy link
Member Author

This should be ready to go, I'll rebase the other open PR on it when merged

@ncdc
Copy link
Contributor

ncdc commented Jun 24, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 24, 2019
@vincepri
Copy link
Member Author

/test pull-cluster-api-test

@k8s-ci-robot k8s-ci-robot merged commit 320b7f4 into kubernetes-sigs:master Jun 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncdc ncdc ncdc left review comments

@detiber detiber Awaiting requested review from detiber

@justinsb justinsb Awaiting requested review from justinsb

Assignees

@ncdc ncdc

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vincepri @k8s-ci-robot @ncdc