-
Notifications
You must be signed in to change notification settings - Fork 400
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
✨ Return error when CRD has multiple versions marked for storage #820
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -142,6 +142,15 @@ func (p *Parser) NeedCRDFor(groupKind schema.GroupKind, maxDescLen *int) { | |
// Otherwise, crd.Spec.Version may point to different CRD versions across different runs. | ||
sort.Slice(crd.Spec.Versions, func(i, j int) bool { return crd.Spec.Versions[i].Name < crd.Spec.Versions[j].Name }) | ||
|
||
for _, err := range ValidateStorageVersionAndServe(crd, groupKind) { | ||
packages[0].AddError(err) | ||
} | ||
|
||
p.CustomResourceDefinitions[groupKind] = crd | ||
} | ||
|
||
func ValidateStorageVersionAndServe(crd apiext.CustomResourceDefinition, groupKind schema.GroupKind) []error { | ||
errs := []error{} | ||
// make sure we have *a* storage version | ||
// (default it if we only have one, otherwise, bail) | ||
if len(crd.Spec.Versions) == 1 { | ||
|
@@ -151,14 +160,18 @@ func (p *Parser) NeedCRDFor(groupKind schema.GroupKind, maxDescLen *int) { | |
hasStorage := false | ||
for _, ver := range crd.Spec.Versions { | ||
if ver.Storage { | ||
// more than one storage, already set it true on previous iteration | ||
if hasStorage { | ||
errs = append(errs, fmt.Errorf("CRD for %s has more than one storage version", groupKind)) | ||
break | ||
} | ||
hasStorage = true | ||
break | ||
} | ||
} | ||
if !hasStorage { | ||
// just add the error to the first relevant package for this CRD, | ||
// since there's no specific error location | ||
packages[0].AddError(fmt.Errorf("CRD for %s has no storage version", groupKind)) | ||
errs = append(errs, fmt.Errorf("CRD for %s has no storage version", groupKind)) | ||
Comment on lines
-161
to
+174
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why was this changed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we pass the package in here and use AddError as before? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vincepri I was following this PR Should I stick with that approach or the one you suggested? I think both should work See the comment belowThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vincepri I wouldn't recommend to pass the package into the function because it would complicate the unit test writing ( need to setup a package loader and make it harder to compare the error string. In addition to that, the package loader would throw more error if the user has not have the package installed) . I would keep the function loosely coupled by not including Please let me know what you think |
||
} | ||
|
||
served := false | ||
|
@@ -171,8 +184,8 @@ func (p *Parser) NeedCRDFor(groupKind schema.GroupKind, maxDescLen *int) { | |
if !served { | ||
// just add the error to the first relevant package for this CRD, | ||
// since there's no specific error location | ||
packages[0].AddError(fmt.Errorf("CRD for %s with version(s) %v does not serve any version", groupKind, crd.Spec.Versions)) | ||
errs = append(errs, fmt.Errorf("CRD for %s with version(s) %v does not serve any version", groupKind, crd.Spec.Versions)) | ||
} | ||
|
||
p.CustomResourceDefinitions[groupKind] = crd | ||
return errs | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we collect all the storage versions in a set and error out with information on where the storage flag is set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vincepri
Wondering how should the map looks like?
map[bool]bool
? The value we are checking are just boolean value with the same typehttps://github.com/kubernetes-sigs/controller-tools/pull/820/files#diff-a205d1d131ac7079cecfe9264fd82b9c7b56f982a42b4085fc4a963734cb9c2cR101-R106
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sets.New[String] should do it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a misunderstanding here.
when you loop through different version, it would be something like
v1
,v2
,v3
, and if we put them all in the set, it would be something likeset{"v1,"v2", "v3"}
. Enough though the version is not duplicated, there are still possible that v1 has storageversion set and v2 has storageversion set. Storage Version is a boolean value, not a string, making it a map would bemap{"v1": true,"v2": true, "v3": false}
, I don't think there is a need to use a set or map.We would only need to check if the boolean is triggered twice, that should be more than enough
Please take a look at the unit test https://github.com/kubernetes-sigs/controller-tools/pull/820/files#diff-a205d1d131ac7079cecfe9264fd82b9c7b56f982a42b4085fc4a963734cb9c2cR100 , the Version is a string and the Storage is a boolean