Skip to content

Add skip-generation and group flag to skip updating specified group crds #2046

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

Conversation

pratikjagrut
Copy link
Contributor

@pratikjagrut pratikjagrut commented Oct 11, 2019

Description of the change:
Add --skip-groups flag.
--skip--groups flag takes a slice of the group names as input, nil by default.

Motivation for the change:
Issue #1633

To test:
operator-sdk generate openapi --skip-groups cache.example.com
For more than one group:
operator-sdk generate openapi --skip-groups="cache.example.com,cache.example.com"

Expected output: Specified group's crds are untouched

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 11, 2019
@joelanford
Copy link
Member

joelanford commented Oct 11, 2019

We've been having discussions about how much flexibility we think makes sense to include in some of our subcommands, so I think there should be some discussion about this particular set of flags.

The original motivation listed in #1633 is to permanently stop regenerating a specific CRD because updates to controller-tools and operator-sdk might change how the CRD is generated even if the types don't change.

I haven't looked, but I'm curious if controller-tools can skip generation for a particular set of types based on the presence (or non-presence) of a comment annotation on the type.

At the very least, the CLI UX proposed here seems too complex to me. I think the flags could be combined into one (e.g. --skip-groups)

@pratikjagrut
Copy link
Contributor Author

We've been having discussions about how much flexibility we think makes sense to include in some of our subcommands, so I think there should be some discussion about this particular set of flags.

The original motivation listed in #1633 is to permanently stop regenerating a specific CRD because updates to controller-tools and operator-sdk might change how the CRD is generated even if the types don't change.

I haven't looked, but I'm curious if controller-tools can skip generation for a particular set of types based on the presence (or non-presence) of a comment annotation on the type.

At the very least, the CLI UX proposed here seems too complex to me. I think the flags could be combined into one (e.g. --skip-groups)

@joelanford issue #1633 is still open and active and so I picked it up and the solution was suggested there itself. The CLI UX is a little bit longer but it meant to be similar to CLI UX of operator-sdk add api --skip-generation. If similarity does not matter then --skip-groups is a good alternative and can be implemented. And I was following that discussion, you mentioned in the comment, where the solution was to implement Makefile, but in such scenarios, I'm not sure how it is more flexible than adding a subcommand.

@camilamacedo86
Copy link
Contributor

camilamacedo86 commented Oct 12, 2019

Hi @pratikjagrut,

In order you have and idea over other the alternative design solutions which would bring more flexibility as @joelanford explains see #1655.

+1 for his suggestion as well to replace --skip-generation --group for --skip-groups in order to make it simpler.

@pratikjagrut
Copy link
Contributor Author

Hi @pratikjagrut,

In order you have and idea over other the alternative design solutions which would bring more flexibility as @joelanford explains see #1655.

+1 for his suggestion as well to replace --skip-generation --group for --skip-groups in order to make it simpler.

Ok, @camilamacedo86 I'll replace --skip-generation --group for --skip-groups. But I'm not completely sure that will this PR be obsolete or will be considered? As @joelanford mentioned of using a Makefile based approach in that discussion he commented earlier.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2019
@pratikjagrut pratikjagrut force-pushed the add.generate.openapi.skip-generation.flag branch from 77d58db to 65d14db Compare October 13, 2019 07:11
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2019
@camilamacedo86
Copy link
Contributor

/test e2e-aws-ansible

@joelanford
Copy link
Member

@pratikjagrut Yeah, I'm hesitant to introduce this TBH. I think a direct invocation of controller-gen with the specific package paths is the direction we should steer users, especially since we'll likely be inheriting kubebuilder's manifest generation method (make manifests)

@estroz and @hasbro17 thoughts this?

@camilamacedo86
Copy link
Contributor

camilamacedo86 commented Oct 16, 2019

Hi @pratikjagrut,

It is great that you could figure out how to do that in the project and is working on to collab with. You did a great work 🥇. However, I am afraid we should close this PR and the issue with the same explanation made in the #1804 (comment).

However, please feel free to pick up any other issue to learn more about this project. See the good+first+issue if you prefer. Also, please let me know if I can help you with.

Are you ok with? Do you mind if we close this one?

c/c @joelanford

@pratikjagrut
Copy link
Contributor Author

Hi @pratikjagrut,

It is great that you could figure out how to do that in the project and is working on to collab with. You did a great work . However, I am afraid we should close this PR and the issue with the same explanation made in the #1804 (comment).

However, please feel free to pick up any other issue to learn more about this project. See the good+first+issue if you prefer. Also, please let me know if I can help you with.

Are you ok with? Do you mind if we close this one?

c/c @joelanford

HI @camilamacedo86,
I agree, we have to do whatever is the best for the project :)
You can close this PR and I will love to contribute more.

@camilamacedo86
Copy link
Contributor

Realy thank you @pratikjagrut for your collab and understanding.
I am closing this now. Please, welcome to collabs with 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants