Add make target for generating crds through controller-gen #629

mattcui · 2021-03-01T08:25:14Z

Changes

This PR is for changing to use controller-gen tool from kubebuilder to generate CRDs

Fixes #579

-->

Submitter Checklist

Includes tests if functionality changed/was added
Includes docs if changes are user-facing
Set a kind label on this PR
Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

Added a script and make target to install controller-gen tool and generate CRDs with this tool

openshift-ci-robot · 2021-03-01T08:25:16Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

mattcui · 2021-03-01T13:08:58Z

I am able to use controller-gen tool to generate CRDs successfully, the name of generated manifests is different with existing ones, the generated name has shipwright.io_ prefix and plural noun like below:

deploy/crds# ls
build.yaml     buildstrategy.yaml         shipwright.io_buildruns.yaml  shipwright.io_buildstrategies.yaml
buildrun.yaml  clusterbuildstrategy.yaml  shipwright.io_builds.yaml     shipwright.io_clusterbuildstrategies.yaml

So the 1st question is whether we need to use these generated names or rename to existing ones? Also there lead to another question about the location/directory where to save these crd files. The operator-sdk 1.x migration doc recommends to follow kubebuilder's convention to save these crds under config/crd/bases, we may still keep as it's for now or move to config/crd/bases for future to moving to operator-sdk 1.x.

I also compared the content of generated manifests with existing ones. There is not much different, there added a new annotation for controller-gen.kubebuilder.io/version in newly generated ones by controller-gen, and an empty status section at the bottom the file (Need to see if it's harmless to us).

# diff build.yaml shipwright.io_builds.yaml
0a1,2
>
> ---
3a6,8
>   annotations:
>     controller-gen.kubebuilder.io/version: v0.4.1
>   creationTimestamp: null
270a276,281
> status:
>   acceptedNames:
>     kind: ""
>     plural: ""
>   conditions: []
>   storedVersions: []

/cc @qu1queee @zhangtbj

zhangtbj · 2021-03-02T06:39:04Z

Thanks Matt!

So the 1st question is whether we need to use these generated names or rename to existing ones?

Can controller-gen tool generate CRD name as our original name by leverage its command parameter directly? If no, I suggest to switch to the new name directly. The second folder problem is similar. I prefer to use a native/default way of the tool (no patch).

BTW, can we add our DEVELOPMENT.md document to tell new developer how to use it?
https://github.com/shipwright-io/build/blob/master/DEVELOPMENT.md

HeavyWombat · 2021-03-03T10:54:27Z

hack/install-controller-gen.sh

+if [ ! -f "${GOPATH}/bin/controller-gen" ] ; then
+    echo "# Installing controller-gen..."
+    go get sigs.k8s.io/controller-tools/cmd/controller-gen@"${CONTROLLER_GEN_VERSION}"
+fi


Until we can use the Go 1.16 improved go get/install that does not fiddle with the current Go modules file, I would suggest to do the installation part in a temporary directory.

Suggested change

if [ ! -f "${GOPATH}/bin/controller-gen" ] ; then

echo "# Installing controller-gen..."

go get sigs.k8s.io/controller-tools/cmd/controller-gen@"${CONTROLLER_GEN_VERSION}"

fi

if [ ! -f "${GOPATH}/bin/controller-gen" ] ; then

echo "# Installing controller-gen..."

pushd "$(mktemp -d)" >/dev/null 2>&1

go get sigs.k8s.io/controller-tools/cmd/controller-gen@"${CONTROLLER_GEN_VERSION}"

popd >/dev/null 2>&1

fi

Thanks, Matthias. I updated the code according to your suggestion, BTW, do you think if it's needed to clean up this temporary dir after installation? Thanks.

mattcui · 2021-03-03T13:43:39Z

Thanks Matt!

So the 1st question is whether we need to use these generated names or rename to existing ones?

Can controller-gen tool generate CRD name as our original name by leverage its command parameter directly? If no, I suggest to switch to the new name directly. The second folder problem is similar. I prefer to use a native/default way of the tool (no patch).

BTW, can we add our DEVELOPMENT.md document to tell new developer how to use it?
https://github.com/shipwright-io/build/blob/master/DEVELOPMENT.md

Yes, I added a simple instruction in this doc.

HeavyWombat

Quick question, I tried it locally and with the proposed GO111MODULE fix it was able to create the custom resource definition files. However, the file paths were different to the ones we currently have:

# New files
deploy/crds/shipwright.io_buildruns.yaml
deploy/crds/shipwright.io_builds.yaml
deploy/crds/shipwright.io_buildstrategies.yaml
deploy/crds/shipwright.io_clusterbuildstrategies.yaml

# Current files
ls -1 deploy/crds/
build.yaml
buildrun.yaml
buildstrategy.yaml
clusterbuildstrategy.yaml

HeavyWombat · 2021-03-04T10:28:46Z

hack/install-controller-gen.sh

+if [ ! -f "${GOPATH}/bin/controller-gen" ] ; then
+    echo "# Installing controller-gen..."
+    pushd "$(mktemp -d)" >/dev/null 2>&1
+    go get sigs.k8s.io/controller-tools/cmd/controller-gen@"${CONTROLLER_GEN_VERSION}"


Just found out that in this particular case we have to make sure that it uses Go Modules otherwise you cannot specify a specific version. Tested it in an empty Docker container and with the environment variable it works for me.

Suggested change

go get sigs.k8s.io/controller-tools/cmd/controller-gen@"${CONTROLLER_GEN_VERSION}"

GO111MODULE=on go get sigs.k8s.io/controller-tools/cmd/controller-gen@"${CONTROLLER_GEN_VERSION}"

+1, just hit the same issue

Thanks, @HeavyWombat , I added.

qu1queee · 2021-03-04T10:59:40Z

@mattcui @HeavyWombat the names where changed in here . I think also the operator-sdk (when doing an operator-sdk generate crds) usually tries to create the files with the same syntax <scheme group>_<crd plural name>.yaml . I´m wondering it those files renames are wrong and if this syntax is a best practice. @imjasonh any opinions on this?, this files rename went via the PR where we change the domain name.

@mattcui one more thing, so I compared the files and I see this will introduce new fields in the CRD definition, as follows:

$ dyff between build.yaml shipwright.io_builds.yaml
     _        __  __
   _| |_   _ / _|/ _|  between github.com/shipwright-io/build/deploy/crds/build.yaml
 / _' | | | | |_| |_       and github.com/shipwright-io/build/deploy/crds/shipwright.io_builds.yaml
| (_| | |_| |  _|  _|
 \__,_|\__, |_| |_|   returned two differences
        |___/

(root level)
+ one map entry added:
  status:
  │ acceptedNames:
  │ │ kind:
  │ │ plural:
  │ conditions: []
  │ storedVersions: []

metadata
  + two map entries added:
    annotations:
    │ controller-gen.kubebuilder.io/version: v0.4.1
    creationTimestamp: null

$ dyff between buildrun.yaml shipwright.io_buildruns.yaml
     _        __  __
   _| |_   _ / _|/ _|  between github.com/shipwright-io/build/deploy/crds/buildrun.yaml
 / _' | | | | |_| |_       and github.com/shipwright-io/build/deploy/crds/shipwright.io_buildruns.yaml
| (_| | |_| |  _|  _|
 \__,_|\__, |_| |_|   returned two differences
        |___/

(root level)
+ one map entry added:
  status:
  │ acceptedNames:
  │ │ kind:
  │ │ plural:
  │ conditions: []
  │ storedVersions: []

metadata
  + two map entries added:
    annotations:
    │ controller-gen.kubebuilder.io/version: v0.4.1
    creationTimestamp: null

$ dyff between buildstrategy.yaml shipwright.io_buildstrategies.yaml
     _        __  __
   _| |_   _ / _|/ _|  between github.com/shipwright-io/build/deploy/crds/buildstrategy.yaml
 / _' | | | | |_| |_       and github.com/shipwright-io/build/deploy/crds/shipwright.io_buildstrategies.yaml
| (_| | |_| |  _|  _|
 \__,_|\__, |_| |_|   returned two differences
        |___/

(root level)
+ one map entry added:
  status:
  │ acceptedNames:
  │ │ kind:
  │ │ plural:
  │ conditions: []
  │ storedVersions: []

metadata
  + two map entries added:
    annotations:
    │ controller-gen.kubebuilder.io/version: v0.4.1
    creationTimestamp: null

$ dyff between clusterbuildstrategy.yaml shipwright.io_clusterbuildstrategies.yaml
     _        __  __
   _| |_   _ / _|/ _|  between github.com/shipwright-io/build/deploy/crds/clusterbuildstrategy.yaml
 / _' | | | | |_| |_       and github.com/shipwright-io/build/deploy/crds/shipwright.io_clusterbuildstrategies.yaml
| (_| | |_| |  _|  _|
 \__,_|\__, |_| |_|   returned two differences
        |___/

(root level)
+ one map entry added:
  status:
  │ acceptedNames:
  │ │ kind:
  │ │ plural:
  │ conditions: []
  │ storedVersions: []

metadata
  + two map entries added:
    annotations:
    │ controller-gen.kubebuilder.io/version: v0.4.1
    creationTimestamp: null

any ideas on why this is happening?

qu1queee

Asking some questions on the current setup, overall this looks good!

mattcui · 2021-03-05T05:52:38Z

@mattcui @HeavyWombat the names where changed in here . I think also the operator-sdk (when doing an operator-sdk generate crds) usually tries to create the files with the same syntax <scheme group>_<crd plural name>.yaml . I´m wondering it those files renames are wrong and if this syntax is a best practice. @imjasonh any opinions on this?, this files rename went via the PR where we change the domain name.

@mattcui one more thing, so I compared the files and I see this will introduce new fields in the CRD definition, as follows:

$ dyff between build.yaml shipwright.io_builds.yaml
     _        __  __
   _| |_   _ / _|/ _|  between github.com/shipwright-io/build/deploy/crds/build.yaml
 / _' | | | | |_| |_       and github.com/shipwright-io/build/deploy/crds/shipwright.io_builds.yaml
| (_| | |_| |  _|  _|
 \__,_|\__, |_| |_|   returned two differences
        |___/

(root level)
+ one map entry added:
  status:
  │ acceptedNames:
  │ │ kind:
  │ │ plural:
  │ conditions: []
  │ storedVersions: []

metadata
  + two map entries added:
    annotations:
    │ controller-gen.kubebuilder.io/version: v0.4.1
    creationTimestamp: null

$ dyff between buildrun.yaml shipwright.io_buildruns.yaml
     _        __  __
   _| |_   _ / _|/ _|  between github.com/shipwright-io/build/deploy/crds/buildrun.yaml
 / _' | | | | |_| |_       and github.com/shipwright-io/build/deploy/crds/shipwright.io_buildruns.yaml
| (_| | |_| |  _|  _|
 \__,_|\__, |_| |_|   returned two differences
        |___/

(root level)
+ one map entry added:
  status:
  │ acceptedNames:
  │ │ kind:
  │ │ plural:
  │ conditions: []
  │ storedVersions: []

metadata
  + two map entries added:
    annotations:
    │ controller-gen.kubebuilder.io/version: v0.4.1
    creationTimestamp: null

$ dyff between buildstrategy.yaml shipwright.io_buildstrategies.yaml
     _        __  __
   _| |_   _ / _|/ _|  between github.com/shipwright-io/build/deploy/crds/buildstrategy.yaml
 / _' | | | | |_| |_       and github.com/shipwright-io/build/deploy/crds/shipwright.io_buildstrategies.yaml
| (_| | |_| |  _|  _|
 \__,_|\__, |_| |_|   returned two differences
        |___/

(root level)
+ one map entry added:
  status:
  │ acceptedNames:
  │ │ kind:
  │ │ plural:
  │ conditions: []
  │ storedVersions: []

metadata
  + two map entries added:
    annotations:
    │ controller-gen.kubebuilder.io/version: v0.4.1
    creationTimestamp: null

$ dyff between clusterbuildstrategy.yaml shipwright.io_clusterbuildstrategies.yaml
     _        __  __
   _| |_   _ / _|/ _|  between github.com/shipwright-io/build/deploy/crds/clusterbuildstrategy.yaml
 / _' | | | | |_| |_       and github.com/shipwright-io/build/deploy/crds/shipwright.io_clusterbuildstrategies.yaml
| (_| | |_| |  _|  _|
 \__,_|\__, |_| |_|   returned two differences
        |___/

(root level)
+ one map entry added:
  status:
  │ acceptedNames:
  │ │ kind:
  │ │ plural:
  │ conditions: []
  │ storedVersions: []

metadata
  + two map entries added:
    annotations:
    │ controller-gen.kubebuilder.io/version: v0.4.1
    creationTimestamp: null

any ideas on why this is happening?

I also posted the diff in my comment , the 1st difference, I think, it's controller-gen that adds itself controller-gen.kubebuilder.io/version: v0.4.1 in the annotation. For the 2nd difference, the code and explanation are here -> https://github.com/kubernetes-sigs/controller-tools/blob/master/pkg/crd/spec.go#L167-L173, I think it's Ok if it's harmless to us.

qu1queee

thanks
/lgtm
/approve

openshift-ci-robot · 2021-03-05T09:21:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qu1queee

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:

~~OWNERS~~ [qu1queee]

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

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Label for when a PR has specified a release note labels Mar 1, 2021

openshift-ci-robot requested review from HeavyWombat and xiujuan95 March 1, 2021 08:25

gabemontero mentioned this pull request Mar 1, 2021

Generate CRD manifests without operator-sdk #579

Closed

mattcui changed the title ~~Add make target for generating crds through controller-gen~~ WIP: Add make target for generating crds through controller-gen Mar 2, 2021

HeavyWombat requested changes Mar 3, 2021

View reviewed changes

mattcui force-pushed the gen_crd branch 2 times, most recently from 04d7466 to beada30 Compare March 3, 2021 13:40

mattcui marked this pull request as ready for review March 3, 2021 13:43

mattcui changed the title ~~WIP: Add make target for generating crds through controller-gen~~ Add make target for generating crds through controller-gen Mar 3, 2021

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 3, 2021

mattcui requested a review from HeavyWombat March 3, 2021 13:44

HeavyWombat requested changes Mar 4, 2021

View reviewed changes

qu1queee self-requested a review March 4, 2021 10:44

qu1queee requested changes Mar 4, 2021

View reviewed changes

Add make target for generating crds through controller-gen

47773bc

mattcui force-pushed the gen_crd branch from beada30 to 47773bc Compare March 5, 2021 05:55

qu1queee mentioned this pull request Mar 5, 2021

Fix CRD file naming convention #643

Merged

4 tasks

qu1queee self-requested a review March 5, 2021 09:19

qu1queee approved these changes Mar 5, 2021

View reviewed changes

openshift-ci-robot assigned qu1queee Mar 5, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2021

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 5, 2021

openshift-merge-robot merged commit fa82dd8 into shipwright-io:master Mar 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add make target for generating crds through controller-gen #629

Add make target for generating crds through controller-gen #629

mattcui commented Mar 1, 2021 •

edited

Loading

openshift-ci-robot commented Mar 1, 2021

mattcui commented Mar 1, 2021 •

edited

Loading

zhangtbj commented Mar 2, 2021

HeavyWombat Mar 3, 2021

mattcui Mar 3, 2021

mattcui commented Mar 3, 2021

HeavyWombat left a comment

HeavyWombat Mar 4, 2021

qu1queee Mar 4, 2021

mattcui Mar 5, 2021

qu1queee commented Mar 4, 2021

qu1queee left a comment

mattcui commented Mar 5, 2021

qu1queee left a comment

openshift-ci-robot commented Mar 5, 2021

	go get sigs.k8s.io/controller-tools/cmd/controller-gen@"${CONTROLLER_GEN_VERSION}"
	GO111MODULE=on go get sigs.k8s.io/controller-tools/cmd/controller-gen@"${CONTROLLER_GEN_VERSION}"

Add make target for generating crds through controller-gen #629

Add make target for generating crds through controller-gen #629

Conversation

mattcui commented Mar 1, 2021 • edited Loading

Changes

Submitter Checklist

Release Notes

openshift-ci-robot commented Mar 1, 2021

mattcui commented Mar 1, 2021 • edited Loading

zhangtbj commented Mar 2, 2021

HeavyWombat Mar 3, 2021

Choose a reason for hiding this comment

mattcui Mar 3, 2021

Choose a reason for hiding this comment

mattcui commented Mar 3, 2021

HeavyWombat left a comment

Choose a reason for hiding this comment

HeavyWombat Mar 4, 2021

Choose a reason for hiding this comment

qu1queee Mar 4, 2021

Choose a reason for hiding this comment

mattcui Mar 5, 2021

Choose a reason for hiding this comment

qu1queee commented Mar 4, 2021

qu1queee left a comment

Choose a reason for hiding this comment

mattcui commented Mar 5, 2021

qu1queee left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Mar 5, 2021

mattcui commented Mar 1, 2021 •

edited

Loading

mattcui commented Mar 1, 2021 •

edited

Loading