-
Notifications
You must be signed in to change notification settings - Fork 92
Improve the building steps and add a target for new CRDs #291
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
Conversation
2f780e1 to
6205ae0
Compare
Signed-off-by: manuelbuil <[email protected]>
6205ae0 to
dcf224f
Compare
README.md
Outdated
|
|
||
| #### Locally | ||
| ### Build | ||
| Run `make build` to build the binary and, opitonally, generate new CRDs if the API changed. We recommend execute `make` to additionally run validation and testing. |
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.
| Run `make build` to build the binary and, opitonally, generate new CRDs if the API changed. We recommend execute `make` to additionally run validation and testing. | |
| Run `make build` to build the binary and, optionally, generate new CRDs if the API changed. We recommend just running `make`, as the default target will also perform validation and testing. |
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
manifests/crd.yaml
Outdated
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 are we checking in a SECOND copy of the CRDs? They are already here: https://github.com/k3s-io/helm-controller/tree/master/pkg/crds/yaml/generated
See also the mechanism that we are using for k3s-io/api, rancher/helm-controller, and rancher/rancher:
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 added it Just because the README talks about ./manifests/crd.yaml. But now thinking about this, it's better if we change the README
That is exactly what I used to generate the existing docs pages - did I forget to check that in? See for example what I added to system-upgrade controller: rancher/system-upgrade-controller@42e0dc3 |
Signed-off-by: manuelbuil <[email protected]>
Signed-off-by: manuelbuil <[email protected]>
|
oh no I did add that to Line 5 in 00cae7b
I just didn't add it to the Dockerfile. |
This PR does several things:
1 - Adds a way to easily generate new CRDs if we change some of the fields in pkg/apis/helm.cattle.io/types.go: it adds a new target in the Makefile (generate-crd) that is considered as a dependency of the build target. It also adds a new target in the Dockerfile which copies over the new crds
2 - Related to 1, this PR moves the installation of controller-gen to the
builderstage because this is where it makes sense to have the new CRDs, as we need them to build helm-controller here3 - Adds
go generatein thebuilderstage and also installscrd-ref-docsbecause we need it as part of the code generation here. I pickedgithub.com/elasticbecause that seems to be the best project right now to get crd-ref-docs as apposed to "ahmetb/gen-crd-api-reference-docs" that is unmaintained4 - Removes "goimports@gopls" installation. We are executing
go fmtandgolangci-lintin the validation step but no goimports5 - Adds manifests/crd.yaml because we are talking about it in the README and improves the README a bit because it was slightly outdated
6 - Updates controller-gen to v0.19.0
7 - Removes gcc and musl-dev from the dev stage as the build is happening in the builder stage