Deprecate 'generate openapi', add 'generate crds' #2276

joelanford · 2019-11-27T18:02:56Z

Description of the change:
This PR deprecates the operator-sdk generate openapi command and adds operator-sdk generate crds

Motivation for the change:
The zz_generated.openapi.go files are rarely required, so the SDK should be less opinionated about creating them for users.

This change also aligns the SDK more closely with kubebuilder, which does not generate zz_generated.openapi.go by default.

camilamacedo86 · 2019-11-27T18:12:50Z

CHANGELOG.md

@@ -12,10 +12,12 @@
 - Updated `pkg/test/e2eutil.WaitForDeployment()` and `pkg/test/e2eutil.WaitForOperatorDeployment()` to successfully complete waiting when the available replica count is _at least_ (rather than exactly) the minimum replica count required. ([#2248](https://github.com/operator-framework/operator-sdk/pull/2248))
 - Replace in the Ansible based operators module tests `k8s_info` for `k8s_facts` which is deprecated. ([#2168](https://github.com/operator-framework/operator-sdk/issues/2168))
 - Upgrade the Ansible version from `2.8` to `2.9` on the Ansible based operators image. ([#2168](https://github.com/operator-framework/operator-sdk/issues/2168))
+- **Breaking change:** Renamed `operator-sdk generate openapi` to `operator-sdk generate crds`. ([#2276](https://github.com/operator-framework/operator-sdk/pull/2276))


I am not sure if we should move forward with. See that kubebuilder is not supporting it and users are asking for here and a track issue from them here which in my understand shows that require it as well.

The first issue is fair; based on feedback in #2042 some users do not use zz_generated.openapi.go, which, on top of it not being necessary for API code unlike zz_generated.deepcopy.go, is why I decided to push for removing it. We should either:

Add documentation on building and using openapi-gen to generate the files yourself, which is fairly straightforward.

Keep generate openapi as a command but move CRD generation into generate crd.

I vote for option 1 since we can offer Go OpenAPI code by wrapping openapi-gen in a Makefile recipe (in the future).

The second issue describes something we do not currently support in API code right now since we use the same tool as kubebuilder to generate CRDs, controller-gen from controller-tools. Generating zz_generated.openapi.go does not change the state of supported annotations for CRD manifest validation in the SDK.

We agreed to make this change because it de-couples CRD generation from OpenAPI spec generation, two different albeit related processes. This change will help users debug errors more easily as well.

Another option would be to add generate crds but keep generate openapi as is and deprecate it. We could hide it from the help output and log a deprecation message when it is called.

Then in a later version it could be removed.

And we could/should still do one of the options @estroz suggested.

@joelanford I like the combination of what you suggested and my first option. The docs referencing Go OpenAPI spec generation (I believe the only documentation doing so is the one being changed in this PR already) should warn that the subcommand is deprecated and users should switch to installing and running openapi-gen themselves if they still want that spec file.

I think the idea of phasing out the old command with a deprecation warning.

@estroz @joelanford we need to make it easy to download and use the openapi-gen command if that is the guidance, we either do it for the user and attempt to install the binary into a known location and give them the option to supply their own openapi-gen.

Could we vendor the openapi-gen command and use it in the operator-sdk command? was that an option already looked?

@shawn-hurley we currently vendor the exposed openapi-gen generator in generate openapi. If we want to install this binary for the user, I suggest this as the first candidate for a Makefile:

openapi-gen: which ./bin/openapi-gen > /dev/null || go build -o ./bin/openapi-gen k8s.io/kube-openapi/cmd/openapi-gen ./bin/openapi-gen ...

Otherwise documentation to the effect of the above script should suffice. I'd rather have documentation than a make recipe.

By the way, our vendoring of the code generation libraries is what causes the GOROOT and panic issues described in this issue: #1854 (comment)

If we direct users to build openapi-gen from source like Eric suggests, we'll help them avoid that issue, at least for the openapi generation (I think it will still be an issue for deepcopy generation).

Alright, updated the PR to keep generate openapi, but to hide it from help and deprecate it.

hack/generate/gen-cli-doc.go

estroz · 2019-12-03T22:18:49Z

doc/sdk-cli-reference.md

@@ -134,6 +134,21 @@ pkg/apis/app/v1alpha1/

 ### openapi


Side note: should we remove this doc entirely now that we auto-generate CLI docs under doc/cli?

Yeah, probably.

@theishshah is there anything else we need to do before we remove it?

If we are just deprecated the command then:
IHMO:

We should not remove/change the docs

We just need to add on top of the func used by it which will be removed the comments that it is deprecated for have the func strikethrough as:

Comment: // Deprecated: xpto

Also, we need to provide a doc with the info for users knowhow them can customize the projects to still be able to gen and use the open API. Will be it done in another PR?

Then, should not be better have a PR to deprecated and one to add the new command?

We want existing users who have written scripts around generate openapi to still be able to use the command while they migrate to generate crds, but new users to use generate crds. By removing docs we can ensure visibility of generate crds without confusing anyone.

The "documentation" explaining deprecation and new behavior is here. We can remove doc/sdk-cli-reference.md in a follow-up PR.

I'm ok with a deprecation comment, like it is used here.

I can put the GoDoc deprecation comments on the functions used for generate openapi but they won't serve much purpose since none of the functions are public. IMO the big reason to add the deprecation comments is to signal to users that the API is changing, but we're the only users of these private functions.

This PR isn't really a Go code API deprecation. It's a deprecation of a CLI subcommand.

Hi @joelanford,

The comments would be helpful to us know what should not be used so long and removed soon. Regards the command, a notice output as it was done here for example would be more appropriate.

c/c @estroz

This PR includes a deprecation notice in the output when generate openapi is run. See here.

hack/generate/gen-cli-doc.go

estroz

/lgtm

@shawn-hurley you're ok with the alternate method to generate zz_generated.openapi.go?

…i' to 'generate crds'

shawn-hurley

This all makes sense to me now thanks!

shawn-hurley · 2019-12-04T15:30:25Z

cmd/operator-sdk/generate/crds.go

+	"github.com/spf13/cobra"
+)
+
+func newGenerateCRDsCmd() *cobra.Command {


can we add a flag for passing in the location of the CRD's?

Once this merges, I'll make an issue to discuss this idea for a follow-up.

camilamacedo86

.

camilamacedo86

/lgtm
/approved

Great 👍

operator-sdk no longer generates the openapi file[1], so remove it from the repo. If we want to continue offering it, we'll need to use openapi-gen directly[2]. [1] operator-framework/operator-sdk#2276 [2] https://github.com/kubernetes/kube-openapi Signed-off-by: James Slagle <jslagle@redhat.com>

joelanford requested review from jmrodri, jmccormick2001, hasbro17, camilamacedo86 and estroz November 27, 2019 18:02

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 27, 2019

camilamacedo86 reviewed Nov 27, 2019

View reviewed changes

estroz reviewed Nov 27, 2019

View reviewed changes

hack/generate/gen-cli-doc.go Outdated Show resolved Hide resolved

estroz mentioned this pull request Dec 2, 2019

internal/generate: align CRD generator with controller-gen #2201

Merged

joelanford force-pushed the remove-zz-generated-openapi branch 3 times, most recently from 9dc62c2 to b2992d9 Compare December 3, 2019 20:24

estroz reviewed Dec 3, 2019

View reviewed changes

hack/generate/gen-cli-doc.go Outdated Show resolved Hide resolved

estroz approved these changes Dec 3, 2019

View reviewed changes

openshift-ci-robot assigned estroz Dec 3, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2019

*: remove zz_generated.openapi.go generation, rename 'generate openap…

857e434

…i' to 'generate crds'

joelanford force-pushed the remove-zz-generated-openapi branch from 77b5b7c to 8d8e787 Compare December 4, 2019 15:00

cmd: keep openapi with deprecation and instructions for migrating

7c75f90

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2019

hack/generate/gen-cli-doc.go: un-revert cli generator

8d8e787

shawn-hurley reviewed Dec 4, 2019

View reviewed changes

joelanford changed the title *: remove zz_generated.openapi.go, rename 'generate openapi' to 'generate crds' Deprecate 'generate openapi', add 'generate crds' Dec 4, 2019

joelanford added a commit to joelanford/operator-sdk that referenced this pull request Dec 4, 2019

CHANGELOG.md: remove outdated change line for operator-framework#2276

b1c48e6

CHANGELOG.md: remove outdated change line for operator-framework#2276

4c0feb8

joelanford force-pushed the remove-zz-generated-openapi branch from b1c48e6 to 4c0feb8 Compare December 4, 2019 20:19

camilamacedo86 reviewed Dec 4, 2019

View reviewed changes

camilamacedo86 approved these changes Dec 5, 2019

View reviewed changes

openshift-ci-robot assigned camilamacedo86 Dec 5, 2019

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

joelanford merged commit 7cc38cc into operator-framework:master Dec 5, 2019

joelanford deleted the remove-zz-generated-openapi branch December 5, 2019 16:36

ebaron mentioned this pull request Feb 19, 2020

Update operator-sdk version cryostatio/cryostat-operator#74

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate 'generate openapi', add 'generate crds' #2276

Deprecate 'generate openapi', add 'generate crds' #2276

joelanford commented Nov 27, 2019 •

edited

Loading

camilamacedo86 Nov 27, 2019

estroz Nov 27, 2019 •

edited

Loading

joelanford Dec 2, 2019

joelanford Dec 2, 2019

estroz Dec 2, 2019

shawn-hurley Dec 2, 2019

estroz Dec 2, 2019

joelanford Dec 3, 2019

joelanford Dec 3, 2019

estroz Dec 3, 2019

joelanford Dec 3, 2019

camilamacedo86 Dec 4, 2019 •

edited

Loading

estroz Dec 4, 2019 •

edited

Loading

joelanford Dec 5, 2019

camilamacedo86 Dec 5, 2019 •

edited

Loading

joelanford Dec 5, 2019

estroz left a comment

shawn-hurley left a comment

shawn-hurley Dec 4, 2019

joelanford Dec 4, 2019

camilamacedo86 left a comment •

edited

Loading

camilamacedo86 left a comment

Deprecate 'generate openapi', add 'generate crds' #2276

Deprecate 'generate openapi', add 'generate crds' #2276

Conversation

joelanford commented Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

estroz Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 Dec 4, 2019 • edited Loading

Choose a reason for hiding this comment

estroz Dec 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 Dec 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estroz left a comment

Choose a reason for hiding this comment

shawn-hurley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

joelanford commented Nov 27, 2019 •

edited

Loading

estroz Nov 27, 2019 •

edited

Loading

camilamacedo86 Dec 4, 2019 •

edited

Loading

estroz Dec 4, 2019 •

edited

Loading

camilamacedo86 Dec 5, 2019 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading