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

internal/scaffold/crd.go: use structural schema for non-go operator CRDs #2275

Merged

Conversation

joelanford
Copy link
Member

Description of the change:
This PR updates the CRD scaffolder for non-go projects to begin using valid structural schema.

Motivation for the change:
Structural schemas are required in v1 CRDs. This change will make it simpler for project maintainers and users to migrate from v1beta1 CRDs to v1 CRDs in the future.

Since ansible and helm v2 do not provide schemas for their roles, playbooks, and charts, and we have no way to easily derive them, we need to continue preserving all CR fields.

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 27, 2019
@@ -15,6 +15,7 @@
package scaffold

import (
goerrors "errors"
Copy link
Contributor

Choose a reason for hiding this comment

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

why create an alias ? Could not it jus be :

Suggested change
goerrors "errors"
"errors"

Copy link
Member Author

Choose a reason for hiding this comment

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

We're already using github.com/pkg/errors in this file, so one of them needs an alias. Rather than changing all of the existing usages, I thought it would be simpler and easier to review to use an alias here.

Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 27, 2019
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

/lgtm /approved

  • Test changes locally
  • The CRD was created with
$ cat deploy/crds/cache.example.com_memcacheds_crd.yaml 
apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
  name: memcacheds.cache.example.com
spec:
  group: cache.example.com
  names:
    kind: Memcached
    listKind: MemcachedList
    plural: memcacheds
    singular: memcached
  scope: Namespaced
  subresources:
    status: {}
  validation:
    openAPIV3Schema:
      type: object
      x-kubernetes-preserve-unknown-fields: true
  versions:
  - name: v1alpha1
    served: true

  • It was applied in the cluster with success as well.

@joelanford joelanford merged commit c134fd6 into operator-framework:master Nov 28, 2019
@joelanford joelanford deleted the non-go-structural-schema branch November 28, 2019 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants