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.

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 controller-gen to tools.go #564

Merged
merged 6 commits into from
Dec 3, 2021

Conversation

mmlb
Copy link
Contributor

@mmlb mmlb commented Dec 3, 2021

Description

Modifies most of kube.mk to be better integrated with tools.go and accompanying make rules.

Why is this needed

Gets rid of recursive make calls (which are considered harmful 😄) by giving make more/better info.
Also removes code that is no longer necessary (and some that never was, envsubst in kube.mk).

How Has This Been Tested?

I ran make generate

@mmlb mmlb requested a review from micahhausler December 3, 2021 20:34
@mmlb mmlb mentioned this pull request Dec 3, 2021
3 tasks
micahhausler
micahhausler previously approved these changes Dec 3, 2021
Copy link
Contributor

@micahhausler micahhausler left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the cleanup. I mostly just copied what kubebuilder dumps out and what was in CAPT

paths=./pkg/apis/... \
crd:crdVersions=v1 \
rbac:roleName=manager-role \
output:crd:dir=./config/crd/bases \
output:webhook:dir=./config/webhook \
webhook
prettier --write ./config/crd/
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:nod: but now I have to fix ci.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be fixed now

micahhausler
micahhausler previously approved these changes Dec 3, 2021
Copy link
Contributor

@micahhausler micahhausler left a comment

Choose a reason for hiding this comment

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

LGTM

mmlb added 4 commits December 3, 2021 17:08
This way we can use the go tool's built in pinning functionality that
is already used in this project.

Signed-off-by: Manuel Mendez <[email protected]>
Next commit is going to add prettier to one of the generated recipes and
it's not available outside of nix-shell here.

Signed-off-by: Manuel Mendez <[email protected]>
No need to have 2 different ways to manage go based tooling, we already have
tools.go based install using simple `go install` calls.

This also has the benefit that make has proper knowledge of the dependency on
the binary and how to generate it so we don't need to do recursive make which
is harmful.

Signed-off-by: Manuel Mendez <[email protected]>
It is no longer needed.

Signed-off-by: Manuel Mendez <[email protected]>
@mmlb mmlb force-pushed the add-controller-gen-to-tools.go branch from a50aec7 to 4ab4f75 Compare December 3, 2021 22:12
mmlb added 2 commits December 3, 2021 17:15
To get more accurate numbers of timing for the actual action.

Signed-off-by: Manuel Mendez <[email protected]>
Copy link
Contributor

@micahhausler micahhausler left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link

codecov bot commented Dec 3, 2021

Codecov Report

Merging #564 (b0eb3c6) into main (5de0768) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #564   +/-   ##
=======================================
  Coverage   34.76%   34.76%           
=======================================
  Files          44       44           
  Lines        3348     3348           
=======================================
  Hits         1164     1164           
  Misses       2085     2085           
  Partials       99       99           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5de0768...b0eb3c6. Read the comment docs.

@mmlb mmlb changed the title Add controller gen to tools.go Add controller-gen to tools.go Dec 3, 2021
@mmlb mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Dec 3, 2021
@ScottGarman ScottGarman self-requested a review December 3, 2021 22:34
@mergify mergify bot merged commit b72ab0b into tinkerbell:main Dec 3, 2021
@mmlb mmlb deleted the add-controller-gen-to-tools.go branch December 3, 2021 22:41
@displague displague added this to the 0.7.0 milestone Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants