internal/generate: align CRD generator with controller-gen #2201
Conversation
estroz
requested review from
jmrodri,
joelanford,
jmccormick2001,
hasbro17 and
camilamacedo86
openshift-ci-robot
added
the
size/XXL
estroz
added
refactoring
size/XXL
|
@estroz: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/hold blocked by #2202 |
openshift-ci-robot
added
the
do-not-merge/hold
cmd/operator-sdk: replace CRD scaffold with CRD generator internal/generate/util: CachedGenerator wraps controller-tools' generator runtime for cached manifest generation internal/generate/crd: CRD-specific generator test/test-framework: update operator with new CRD generator internal/util/fileutil: DotPath prepends "./" or ".\" to a path depending on OS
test/test-framework: use storage version annotation in APIs
estroz
force-pushed
the
align-crd-generator-controller-gen
branch
from
02e2019 to
8827ac5
Compare
internal/generate/crd/crd.go
Outdated
|
|
||
| // NewGo returns a CRD generator configured to generate a | ||
| // CustomResourceDefintion manifest from scratch using data in resource. | ||
| func NewNonGo(inputDir, outputDir string, resource scaffold.Resource) genutil.Generator { |
There was a problem hiding this comment.
WDYT about it is a NewGenerator and the info if is or not a go project be an attribute of the Generator object?
There was a problem hiding this comment.
I'm going to think about this a bit more. We're effectively doing that with current code using projutil.IsOperatorGo(), I just don't like using booleans as exported function parameters when I don't have to.
test/test-framework/deploy/crds/cache.example.com_memcachedrs_crd.yaml
Outdated
Show resolved
Hide resolved
estroz
force-pushed
the
align-crd-generator-controller-gen
branch
from
15515b7 to
cb33ad1
Compare
…h controller-tools'
estroz
force-pushed
the
align-crd-generator-controller-gen
branch
from
1c2e07e to
6820652
Compare
| ``` | ||
|
|
||
| **Notes:** | ||
| - - Your CRD *must* specify exactly one [storage version][crd-storage-version]. Use the `+kubebuilder:storageversion` [marker][crd-markers] to indicate the GVK that should be used to store data by the API server. This marker should be in a comment above your `Memcached` type. |
There was a problem hiding this comment.
I noticed that without manually adding +kubebuilder:storageversion in advance, operator-sdk add api errors out when another version is added to an existing CRD with the same group & kind.
This problem may be out-of-scope for this PR, but something we may want to think about whether there's a more graceful way to handle from a user experience perspective.
Here's the error I get:
$ operator-sdk generate crds
INFO[0000] Running CRD generator.
example.com/go-operator/pkg/apis/example/v1:-: CRD for GoOperator.example.com has no storage version
Error: error generating CRDs from APIs in pkg/apis: error generating CRD manifests: error running CRD generator: not all generators ran successfully
Usage:
operator-sdk generate crds [flags]
Flags:
-h, --help help for crds
Global Flags:
--verbose Enable verbose logging
It's a good hint, but could be improved by telling the user how to fix the problem. Can we put this note into the error message, nearly verbatim and skip outputting the usage?
There was a problem hiding this comment.
Also, I just noticed that it's possible to define +kubebuilder:storageversion in both versions of a CRD's types.go files and controller-gen happily creates a CRD where storage: true exists in both versions. That should also be an error right?
There was a problem hiding this comment.
👍 on not printing usage. By note do you mean:
example.com/go-operator/pkg/apis/example/v1:-: CRD for GoOperator.example.com has no storage version
Two CRD versions with storage: true is an error, but controller-tools does not consider this an error during generation. I will submit an issue if one does not exist already upstream.
There was a problem hiding this comment.
Oh I meant based on the note that you have in the user guide:
Something like:
Your CRD must specify exactly one storage version. Use the
+kubebuilder:storageversionmarker to indicate the GVK that should be used to store data by the API server. This marker should be in a comment above your CRDs root storage type in_types.go.See https://book.kubebuilder.io/multiversion-tutorial/api-changes.html#storage-versions
There was a problem hiding this comment.
Generator errors are not returned by Runtime.Run() so we can't check if the error returned pertains to no +kubebuilder:storageversion annotation. How about linking https://book.kubebuilder.io/reference/generating-crd.html in an error message for Runtime.Run() errors? This should cover this and other errors, but isn't as specific.
|
Oh also, is the code coverage decrease related to this PR? If so, is it possible to bump that back up? |
internal: remove unused Config method
|
New changes are detected. LGTM label has been removed. |
Description of the change:
Motivation for the change: This PR aligns the CRD generator with controller-gen's. By using the same option strings one would pass to
controller-gento configure the CRD generator, we can easily transition this command to a Makefile, as well as more easily debug CRD generation errors.Note that a resource is no longer required to run the generator for Go projects. It will update each CRD that corresponds to an API. For non-go projects a resource is still required.
Closes #2042