-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: check pre-requisites on OSSM plugin init #7
Conversation
Also, I added the latest SMCP entire CRD - let me know if I should add a truncated one instead. |
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.
Great job! I added a few suggestions, but overall it's solid.
pkg/kfapp/ossm/k8s_resources.go
Outdated
// CreateSMCP uses dynamic client to create a dummy SMCP for testing | ||
func (o *OssmInstaller) CreateSMCP(namespace string, smcpObj *unstructured.Unstructured) error { |
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 would move it to test code as it's only used for testing purposes.
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.
68bb271 done here
err := ossmInstaller.CheckForCRD(crdGroup, crdVersion, crdResource) | ||
|
||
// then | ||
Expect(err).To(HaveOccurred()) |
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.
Maybe it would be worth being more specific and also check for message or parts of 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.
good call, added here 06de003
In odh-project-controller, we fetch it as part of |
Co-authored-by: Bartosz Majsak <[email protected]>
Co-authored-by: Bartosz Majsak <[email protected]>
Co-authored-by: Bartosz Majsak <[email protected]>
Co-authored-by: Bartosz Majsak <[email protected]>
Co-authored-by: Bartosz Majsak <[email protected]>
I took a look at how CRDs are fetched for project-controller - thanks for linking that. Since I want to get the Dashboard Config patching fixed today I've created another issue to handle this in the future here. PTAL and let me know if that's ok. |
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.
One last thing and we are good to go :)
* add prereq checks to init, add tests --------- Co-authored-by: Bartosz Majsak <[email protected]>
* add prereq checks to init, add tests --------- Co-authored-by: Bartosz Majsak <[email protected]>
Creating tests was a bit difficult for the SMCP specifically - please let me know if you see any ways that that process of simulating an SMCP install can be improved.