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

Bug fix:'operator-sdk add crd' doesn't work after add kubebuilder tags #1660

Merged
merged 5 commits into from
Aug 9, 2019

Conversation

xm2
Copy link
Contributor

@xm2 xm2 commented Jul 10, 2019

Description of the change:
Bug fix: 'operator-sdk add crd' doesn't work after add kubebuilder tags in _types.go

Motivation for the change:

  1. In golang operator, after add kubebuilder tags in _types.go files, e.g. validation tag or scale subresource tag,
type AppServiceSpec struct {
	// INSERT ADDITIONAL SPEC FIELDS - desired state of cluster
	// Important: Run "operator-sdk generate k8s" to regenerate code after modifying this file
	// Add custom validation using kubebuilder tags: https://book-v1.book.kubebuilder.io/beyond_basics/generating_crd.html
+	// +kubebuilder:validation:Maximum=100
+	// +kubebuilder:validation:Minimum=1
+	Replicas *int32 `json:"size,omitempty"`
}

// AppServiceStatus defines the observed state of AppService
// +k8s:openapi-gen=true
type AppServiceStatus struct {
	// INSERT ADDITIONAL STATUS FIELD - define observed state of cluster
	// Important: Run "operator-sdk generate k8s" to regenerate code after modifying this file
	// Add custom validation using kubebuilder tags: https://book-v1.book.kubebuilder.io/beyond_basics/generating_crd.html
+	Replicas int32 `json:"replicas,omitempty"`
}

// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

// AppService is the Schema for the appservices API
// +k8s:openapi-gen=true
// +kubebuilder:subresource:status
+// +kubebuilder:subresource:scale:specpath=.spec.size,statuspath=.status.replicas
type AppService struct {
	metav1.TypeMeta   `json:",inline"`
	metav1.ObjectMeta `json:"metadata,omitempty"`

	Spec   AppServiceSpec   `json:"spec,omitempty"`
	Status AppServiceStatus `json:"status,omitempty"`
}
  1. run "operator-sdk generate k8s"
  2. delete "deploy/crds/app_v1alpha1_appservice_crd.yaml"
  3. run operator-sdk add crd --api-version=app.example.com/v1alpha1 --kind=AppService to re-generated crd yaml based on new kubebuilder tag, it failed:
$ rm deploy/crds/app_v1alpha1_appservice_crd.yaml
$ operator-sdk add crd  --api-version=app.example.com/v1alpha1 --kind=AppService
INFO[0000] Generating Custom Resource Definition (CRD) version app.example.com/v1alpha1 for kind AppService. 
Error: crd scaffold failed: (PROJECT file missing in dir /home/centos/go/src/app-operator)
Usage:
  operator-sdk add crd [flags]

Flags:
      --api-version string   Kubernetes apiVersion and has a format of $GROUP_NAME/$VERSION (e.g app.example.com/v1alpha1)
  -h, --help                 help for crd
      --kind string          Kubernetes CustomResourceDefintion kind. (e.g AppService)

Global Flags:
      --verbose   Enable verbose logging

Root cause analysis:
From investigation, this error is reported when the Repo in input.Config is not set and no "PROJECT" file in project root path when call ValidateAndInitFields() in controller-tools/pkg/crd/generator/generator.go.
To fix the error, either add "Repo" in input.Config, or, add "PROJECT"file in project root path.
BUT "PROJECT" is a file in kubebuilder generated project, it doesn't exist in operator-sdk project.

So in the code change, fix the bug by adding "Repo" in input.Config.

Test Result
After the fix, the _crd.yaml can be generated correctly with new "kubebuilder" tags:

$ rm deploy/crds/app_v1alpha1_appservice_crd.yaml
$ operator-sdk add crd  --api-version=app.example.com/v1alpha1 --kind=AppService
INFO[0000] Generating Custom Resource Definition (CRD) version app.example.com/v1alpha1 for kind AppService. 
INFO[0005] Created deploy/crds/app_v1alpha1_appservice_crd.yaml 
INFO[0005] RBAC rules in deploy/role.yaml already up to date for the resource (app.example.com/v1alpha1, AppService) 
INFO[0005] CRD generation complete.                     
$ git diff deploy/crds/app_v1alpha1_appservice_crd.yaml
diff --git a/deploy/crds/app_v1alpha1_appservice_crd.yaml b/deploy/crds/app_v1alpha1_appservice_crd.yaml
index 387e731..e01df42 100644
--- a/deploy/crds/app_v1alpha1_appservice_crd.yaml
+++ b/deploy/crds/app_v1alpha1_appservice_crd.yaml
@@ -11,6 +11,9 @@ spec:
     singular: appservice
   scope: Namespaced
   subresources:
+    scale:
+      specReplicasPath: .spec.size
+      statusReplicasPath: .status.replicas
     status: {}
   validation:
     openAPIV3Schema:
@@ -28,8 +31,26 @@ spec:
         metadata:
           type: object
         spec:
+          properties:
+            size:
+              description: 'INSERT ADDITIONAL SPEC FIELDS - desired state of cluster
+                Important: Run "operator-sdk generate k8s" to regenerate code after
+                modifying this file Add custom validation using kubebuilder tags:
+                https://book-v1.book.kubebuilder.io/beyond_basics/generating_crd.html'
+              format: int32
+              maximum: 100
+              minimum: 1
+              type: integer
           type: object
         status:
+          properties:
+            replicas:
+              description: 'INSERT ADDITIONAL STATUS FIELD - define observed state
+                of cluster Important: Run "operator-sdk generate k8s" to regenerate
+                code after modifying this file Add custom validation using kubebuilder
+                tags: https://book-v1.book.kubebuilder.io/beyond_basics/generating_crd.html'
+              format: int32
+              type: integer
           type: object
   version: v1alpha1
   versions:

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 10, 2019
@openshift-ci-robot
Copy link

Hi @xm2. Thanks for your PR.

I'm waiting for a operator-framework or openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 10, 2019
@lilic lilic requested a review from estroz July 12, 2019 14:19
@lilic
Copy link
Member

lilic commented Jul 12, 2019

cc @estroz ^

@estroz
Copy link
Member

estroz commented Jul 12, 2019

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 12, 2019
@AlexNPavel
Copy link
Contributor

AlexNPavel commented Jul 12, 2019

Does running operator-sdk generate openapi work? I believe that command should correctly regenerate the CRDs.

EDIT: The reason that command works is because it sets the repo field of input.Config, which is what's being added to cmd/add in this PR.

@estroz
Copy link
Member

estroz commented Jul 12, 2019

@xm2 can you rebase onto/merge with master? We made a change in master that affects CI on older branches.

@hasbro17
Copy link
Contributor

@xm2 Since this is a bug fix, can you please add a line to the Bugfix section of the CHANGELOG in the same format as the other changelogs.
https://github.com/operator-framework/operator-sdk/blob/master/CHANGELOG.md#bug-fixes

@estroz PTAL.

Copy link
Contributor

@AlexNPavel AlexNPavel left a comment

Choose a reason for hiding this comment

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

LGTM

CHANGELOG.md Outdated Show resolved Hide resolved
Co-Authored-By: Eric Stroczynski <estroczy@redhat.com>
@estroz estroz added the kind/bug Categorizes issue or PR as related to a bug. label Aug 8, 2019
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 Aug 8, 2019
@estroz
Copy link
Member

estroz commented Aug 8, 2019

@xm2 you'll have to merge your branch with master to fix the CI error unfortunately.

@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2019
@xm2
Copy link
Contributor Author

xm2 commented Aug 9, 2019

@xm2 you'll have to merge your branch with master to fix the CI error unfortunately.

OK. @estroz, just synced my branch with master, all CI checks are OK now.

@estroz
Copy link
Member

estroz commented Aug 9, 2019

@xm2 awesome, and thanks for the PR!

@estroz estroz merged commit 9d6ffdd into operator-framework:master Aug 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants