-
Notifications
You must be signed in to change notification settings - Fork 78
Refactor validate CRD to fix conversion error #12
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
Refactor validate CRD to fix conversion error #12
Conversation
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.
LGTM. I came to the same conclusion from my test
/hold |
b99702e
to
5a81c8f
Compare
1. Fix conversion error due to incompatible type 1. Refactor validate CRD code to reduce duplication 2. Support v1.CRD type Signed-off-by: Vu Dinh <vdinh@redhat.com>
5a81c8f
to
a7119ae
Compare
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.
Change looks good to me. It also looks like a very easy candidate for an accompanying unit test.
Signed-off-by: Vu Dinh <vdinh@redhat.com>
3973850
to
fab55ed
Compare
|
||
results := []errors.ManifestResult{} | ||
switch tt.version { | ||
case "v1": |
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'd make this default rather than the beta version. But really only if you want to.
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.
My comment was only a weak suggestion. LGTM
Signed-off-by: Vu Dinh vdinh@redhat.com