Skip to content
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

Autogeneration of CRDs. #1240

Merged
merged 4 commits into from
Jan 7, 2020
Merged

Autogeneration of CRDs. #1240

merged 4 commits into from
Jan 7, 2020

Conversation

porridge
Copy link
Member

@porridge porridge commented Jan 2, 2020

Before merging:

What this PR does / why we need it:

With this change, the YAML files in config/crds/ are auto-generated from the structs in pkg/apis/, which in turn become the single source of truth for both Go code and CRDs.

  • add controller-gen command to the generate target
  • change annotations to relax validation somewhat, to match current
    usage patterns, and in some cases fix tests to conform to newly added
    validation
  • re-generate files. The changes to CRDs fall in the the following
    categories:
    • descriptions copied from the comments in structs (the vast majority of
      this change)
    • validation tree expanded deeper (i.e. actual allowed properties are
      listed rather than just saying that "this is an object") and the
      required properties brought up to date with the structs
    • some fields added explicitly with the same value as they have by
      default when omitted (singular, listKind, versions)
    • added controller-gen.kubebuilder.io/version annotation
    • the controller-tools.k8s.io: "1.0" label removed from the Test*
      types
    • some missing properties added in the Test* types (commands,
      kindContainers, kindNodeCache)

Fixes #862

Signed-off-by: Marcin Owsiany marcin@owsiany.pl

Signed-off-by: Marcin Owsiany <marcin@owsiany.pl>
* add controller-gen command to the generate target
* change annotations to relax validation somewhat, to match current
  usage patterns, and in some cases fix tests to conform to newly added
  validation
* re-generate files. The changes to CRDs fall in the the following
  categories:
  - descriptions copied from the comments in structs (the vast majority of
    this change)
  - validation tree expanded deeper (i.e. actual allowed properties are
    listed rather than just saying that "this is an object") and the
    `required` properties brought up to date with the structs
  - some fields added explicitly with the same value as they have by
    default when omitted (`singular`, `listKind`, `versions`)
  - added `controller-gen.kubebuilder.io/version` annotation
  - the `controller-tools.k8s.io: "1.0"` label removed from the `Test*`
    types
  - some missing properties added in the `Test*` types (`commands`,
    `kindContainers`, `kindNodeCache`)

Signed-off-by: Marcin Owsiany <marcin@owsiany.pl>
@@ -3,44 +3,161 @@
apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
annotations:
controller-gen.kubebuilder.io/version: v0.2.4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have to have this annotation as it will create diff on CRDs every time we update?

Copy link
Member Author

@porridge porridge Jan 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming by "every time we update" you mean bumping the controller-gen dependency. Do you think we'll be doing this more often than every few months?

It's being added unconditionally, so we have the following options:

  1. leave it as is - after bumping the dependency we can update this everywhere automatically with make generate && grc -c conf.gotest go test -tags integration ./pkg/kudoctl/cmd -v -mod=readonly --update -run TestInitCmd_yamlOutput
  2. If you think this is still too much of a hassle, add a post-processing step to make generate which will delete this annotation with something like https://github.com/mikefarah/yq

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not big deal, but we recently removed annotation of controller-tools: 1.0 from our CRDs which was something similar in my head so I was just wondering if we need this one. I don't think it's worth the hassle of removing it though :) Thanks for listing the options though

description: "PlanStatus is representing status of a plan \n These
are valid states and transitions \n | Never
executed | | v
| Error |<------>| Pending | ^ |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eh, this does not look good

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, although I'm assuming nobody will be treating this file as documentation, all the more these are our "internal" CRDs, so I don't know how much energy it makes sense to invest to improve how this ASCII-art looks here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree about internal documentation. But this still looks awkward. Should we maybe separate this diagram from the PlanStatus doc so it simply doesn't appear here? Wdyt @porridge ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zen-dog yes, that was one option I had in mind.
The other would be to leave it as is, and then look at some tools which would take the CRDs as input and generate nice user-friendly documentation. In that case this ASCII-art might survive and actually be useful? Perhaps this is something worth looking at when we work on docs next week?

Copy link
Contributor

@alenkacz alenkacz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had just those two small nits but this looks great :)

@ANeumann82
Copy link
Member

While you're touching this: The CRDs are currently residing in:
config/crds/kudo.dev_instances.yaml
for example. Would it be a bit deal to move them to
config/crds/v1beta1/kudo.dev_instances.yaml
?

At some point we're going to have multiple CRD versions in the code base, it would be nice to have this already prepared. If it's a big change, i'll put it on my list for later.

@porridge
Copy link
Member Author

porridge commented Jan 7, 2020

@ANeumann82
Copy link
Member

@porridge Right, you're correct. Forgot about that. In that case, never mind :)

@porridge porridge changed the base branch from dash-dash-dash to master January 7, 2020 13:56
@porridge porridge merged commit 7d84119 into master Jan 7, 2020
@porridge porridge deleted the controller-gen1 branch January 7, 2020 14:11
ANeumann82 pushed a commit that referenced this pull request Feb 13, 2020
* add controller-gen command to the generate target
* change annotations to relax validation somewhat, to match current
  usage patterns, and in some cases fix tests to conform to newly added
  validation
* re-generate files. The changes to CRDs fall in the the following
  categories:
  - descriptions copied from the comments in structs (the vast majority of
    this change)
  - validation tree expanded deeper (i.e. actual allowed properties are
    listed rather than just saying that "this is an object") and the
    `required` properties brought up to date with the structs
  - some fields added explicitly with the same value as they have by
    default when omitted (`singular`, `listKind`, `versions`)
  - added `controller-gen.kubebuilder.io/version` annotation
  - the `controller-tools.k8s.io: "1.0"` label removed from the `Test*`
    types
  - some missing properties added in the `Test*` types (`commands`,
    `kindContainers`, `kindNodeCache`)

Signed-off-by: Marcin Owsiany <marcin@owsiany.pl>
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make sure CRDs are in sync with their counterpart Go structs
4 participants