-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
internal/generate: align CRD generator with controller-gen #2201
internal/generate: align CRD generator with controller-gen #2201
Conversation
@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 |
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
15515b7
to
cb33ad1
Compare
…h controller-tools'
1c2e07e
to
6820652
Compare
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.
/lgtm
Just a couple of questions/observations
Do we need a CHANGELOG entry for this? It strikes me as more of a refactoring with no user-facing changes, but wanted to double check at least.
``` | ||
|
||
**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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:storageversion
marker 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-gen
to 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