Skip to content
This repository has been archived by the owner on Sep 12, 2023. It is now read-only.

pr 172 breaks training-operator #175

Open
zw0610 opened this issue Nov 11, 2021 · 6 comments
Open

pr 172 breaks training-operator #175

zw0610 opened this issue Nov 11, 2021 · 6 comments

Comments

@zw0610
Copy link
Member

zw0610 commented Nov 11, 2021

#172 breaks training-operator when installing crd

reproduce steps:

  1. change go.mod and go.sum
diff --git a/go.mod b/go.mod
index 58f089d7..d491f115 100644
--- a/go.mod
+++ b/go.mod
@@ -5,7 +5,7 @@ go 1.17
 require (
        github.com/go-logr/logr v0.3.0
        github.com/go-openapi/spec v0.20.3
-       github.com/kubeflow/common v0.3.7
+       github.com/kubeflow/common v0.4.1-0.20211106174700-a1f26a9cd2f5
        github.com/onsi/ginkgo v1.14.1
        github.com/onsi/gomega v1.10.2
        github.com/prometheus/client_golang v1.10.0
  1. run make manifets
On branch master
Your branch is behind 'origin/master' by 1 commit, and can be fast-forwarded.
  (use "git pull" to update your local branch)

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   go.mod
	modified:   go.sum
	modified:   manifests/base/crds/kubeflow.org_mxjobs.yaml
	modified:   manifests/base/crds/kubeflow.org_pytorchjobs.yaml
	modified:   manifests/base/crds/kubeflow.org_tfjobs.yaml
	modified:   manifests/base/crds/kubeflow.org_xgboostjobs.yaml

no changes added to commit (use "git add" and/or "git commit -a")
  1. run make install
which: no golangci-lint in (/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/root/bin:/usr/local/go/bin)
/root/test/training-operator/bin/controller-gen "crd:trivialVersions=true,preserveUnknownFields=false,generateEmbeddedObjectMeta=true" rbac:roleName=manager-role webhook paths="./pkg/apis/..." output:crd:artifacts:config=manifests/base/crds
go: creating new go.mod: module tmp
Downloading sigs.k8s.io/kustomize/kustomize/[email protected]
go: downloading sigs.k8s.io/kustomize/kustomize/v3 v3.8.7
...
go get: added sigs.k8s.io/yaml v1.2.0
/root/test/training-operator/bin/kustomize build manifests/base/crds | kubectl apply -f -
Error from server (Invalid): error when creating "STDIN": CustomResourceDefinition.apiextensions.k8s.io "mxjobs.kubeflow.org" is invalid: spec.validation.openAPIV3Schema.properties[status].properties[conditions].items.x-kubernetes-map-type: Invalid value: "null": must be atomic as item of a list with x-kubernetes-list-type=set
Error from server (Invalid): error when creating "STDIN": CustomResourceDefinition.apiextensions.k8s.io "pytorchjobs.kubeflow.org" is invalid: spec.validation.openAPIV3Schema.properties[status].properties[conditions].items.x-kubernetes-map-type: Invalid value: "null": must be atomic as item of a list with x-kubernetes-list-type=set
Error from server (Invalid): error when creating "STDIN": CustomResourceDefinition.apiextensions.k8s.io "tfjobs.kubeflow.org" is invalid: spec.validation.openAPIV3Schema.properties[status].properties[conditions].items.x-kubernetes-map-type: Invalid value: "null": must be atomic as item of a list with x-kubernetes-list-type=set
Error from server (Invalid): error when creating "STDIN": CustomResourceDefinition.apiextensions.k8s.io "xgboostjobs.kubeflow.org" is invalid: spec.validation.openAPIV3Schema.properties[status].properties[conditions].items.x-kubernetes-map-type: Invalid value: "null": must be atomic as item of a list with x-kubernetes-list-type=set
make: *** [Makefile:83: install] Error 1
@zw0610
Copy link
Member Author

zw0610 commented Nov 11, 2021

anyone knows how to fix it?

@gaocegege
Copy link
Member

cc @pugangxa

@pugangxa
Copy link
Contributor

pugangxa commented Nov 11, 2021

There's bunch of this errors like kubernetes/kubernetes#91395 when searched and occur even after I changed to listType=map in #174 . Either with a type of set or map will add the item as required in schema.
Investigated and seems the solution to add defaulter to default the values for the map key. But I don't think it's reasonable to add default value to the condition.
Also since there's a lot of this kind of API rule violation which is just a warning but the open api definition is still generated. I will rollback it in #174

@pugangxa
Copy link
Contributor

pugangxa commented Nov 11, 2021

@alculquicondor
Copy link
Contributor

Have you tried adding // +optional and omitempty?

@pugangxa
Copy link
Contributor

Have you tried adding // +optional and omitempty?

Did a quick test by adding // +optional and omitempty, although this can remove condition from required in the CRD, but still report this error when applying.
I think there's lots of this api rule violation(before I think it's only this one) during compiling common so we can leave this for now and address together if needed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants