cmd/operator-sdk/generate/crds.go: check for existence of pkg/apis
#3091
Conversation
joelanford
force-pushed
the
generate-crds-non-go
branch
from
4f0293f
to
140b81c
Compare
|
@joelanford this is technically a bugfix right? Would be worth adding a fragment. |
cmd/operator-sdk/generate/crds.go
Outdated
| // improper CLI usage. | ||
| if err := genutil.CRDGen(crdVersion); err != nil { | ||
| log.Fatal(err) | ||
| switch t := projutil.GetOperatorType(); t { |
There was a problem hiding this comment.
Rather than switching on this, can we define a new switch that checks for the presence of pkg/apis and go.mod, as it seems that those are the only required files for generation to succeed. This will make it so that users that provide those files can still use these functions in non-Go-based operators
There was a problem hiding this comment.
Would they have pkg/apis if cmd/manager/main.go wasn't also present?
There was a problem hiding this comment.
currently they would have to create pkg/apis themselves (or do something hacky like touch main.go before running it so that it's detected as a go-type project), but if there is a pkg/apis present then it works whether or not the rest of the scaffolded go code is there
There was a problem hiding this comment.
I'm not sure if there are any users out there who have figured out that this works, but if there are it doesn't seem to be technically necessary for us to block it off. We don't need to explicitly support it (and if we want to should probably add options to the Ansible scaffolding to generate these files), but if a user wants to generate types in this way I see no reason to add additional barriers.
There was a problem hiding this comment.
IMO since we do not support the gen k8s and cRD this shows fine.
However, if we would like to follow up @fabianvf suggestion we can just call the genutil.CRDGen for Ansible and Helm and print an alert informing that is not supported instead of the block as well.
|
There's a bug associated with this https://bugzilla.redhat.com/show_bug.cgi?id=1838881. Once this PR merges we need to move the bug to MODIFIED. |
| @@ -17,11 +17,12 @@ package generate | |||
| import ( | |||
There was a problem hiding this comment.
- missing fragment before the merge (blocker to merge)
- Consider the approach to alert that is not supported instead of the block as suggested by @fabianvf
otherwise
/lgtm
/approve
joelanford
force-pushed
the
generate-crds-non-go
branch
from
140b81c
to
66bca03
Compare
|
New changes are detected. LGTM label has been removed. |
joelanford
force-pushed
the
generate-crds-non-go
branch
from
66bca03
to
0e08657
Compare
joelanford
changed the title
pkg/apis
|
HI @joelanford, Shows that it is missing just a rebase with the master to get merged. |
openshift-ci-robot
added
the
needs-rebase
joelanford
force-pushed
the
generate-crds-non-go
branch
from
0e08657
to
652ca2c
Compare
openshift-ci-robot
removed
the
needs-rebase
|
@fabianvf @estroz @camilamacedo86 Updated per @fabianvf's suggestion and rebased to latest |
camilamacedo86
added
approved
Description of the change:
Emit error message when using
operator-sdk generate crdsandpkg/apisdirectory does not exist.Motivation for the change:
operator-sdk generate crdsis generally only supported for Go projects, which have apkg/apisdirectory by default. Non-Go projects could use this command, but only if they manually createpkg/apisand define Go types for their APIs.