-
Notifications
You must be signed in to change notification settings - Fork 140
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: Mapping component versions to RHOAI releases #1299
base: incubation
Are you sure you want to change the base?
feat: Mapping component versions to RHOAI releases #1299
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4b85fbc
to
d29af02
Compare
It's still draft, isn't it? |
@Sara4994 I'd like to see this kind of steps to be performed only once i.e. at initialization time as component version won't change during the lifetime of the operator. |
d29af02
to
1038ec1
Compare
1038ec1
to
35692d9
Compare
This PR can't be merged just yet 😢Please run For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/11404123045 |
3a935ab
to
210bde1
Compare
210bde1
to
cfea723
Compare
This PR can't be merged just yet 😢Please run For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/11445735405 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## incubation #1299 +/- ##
=============================================
Coverage ? 19.09%
=============================================
Files ? 30
Lines ? 3368
Branches ? 0
=============================================
Hits ? 643
Misses ? 2656
Partials ? 69 ☔ View full report in Codecov by Sentry. |
cfea723
to
8181671
Compare
This PR can't be merged just yet 😢Please run For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/11445853071 |
f3bb873
to
21bdd88
Compare
e311476
to
7dcd0e4
Compare
do not checkin |
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.
some small parts, otherwise i am fine with the changes.
controllers/datasciencecluster/datasciencecluster_controller.go
Outdated
Show resolved
Hide resolved
73bdce6
to
6600709
Compare
04bee7b
to
12889bd
Compare
/test opendatahub-operator-e2e |
This PR can't be merged just yet 😢Please run For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/11579205049 |
68e228a
to
89d0ef9
Compare
@ykaliuta @zdtsw @VaishnaviHire @lburgazzoli I have tweaked Reason: few components have got multiple upstreams, reading release info of multiple upstreams from an env would be tricky, whereas with yaml we could group the info based on the upstream, and it would be easy to fetch the details at our end. So made it as yaml file. Please feel free to share your thoughts. |
/cc @grdryn |
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.
/hold
(at least until ./manager is removed from the patch set)
383fcf6
to
99da1f1
Compare
/unhold It looks like, for better or worse, the merge automation in this repo will squash all commits in a PR into a single commit, when merging, so the fact that the manager binary was added and then removed in separate commits won't matter: those should cancel each other out and it won't exist in the final history. |
99da1f1
to
0dd61dc
Compare
0dd61dc
to
0a8ed69
Compare
controllers/status/status.go
Outdated
|
||
// GetReleaseVersion reads odh_metadata.yaml file and parses release information. | ||
// If version is not set or set to "", return empty {}. | ||
func GetReleaseVersion(defaultManifestPath string, componentName string) ComponentStatus { |
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 GetReleaseVersion
should return a []ComponentReleaseStatus
not the entire ComponentStatus
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.
@lburgazzoli but incase of yaml file not present in the components repo, we discussed to show empty struct against component names. such error conditions are handled in the functions itself so as returning empty struct. When returning []ComponentReleaseStatus
, will have to handle error cases in components.
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.
you can return an empty list, if you sue omitempty
in the tags, then it won't not show up
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.
@lburgazzoli yep, have made this changes now. :)
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
GetReleaseVersion
should return a[]ComponentReleaseStatus
not the entireComponentStatus
If it returns ReleaseStatus, why it's still called GetReleaseVersion? (asked @Sara4994 already)
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.
@ykaliuta renamed it now. thanks
componentReleaseStatus := make([]ComponentReleaseStatus, 0) | ||
|
||
yamlData, err := os.ReadFile(filepath.Join(defaultManifestPath, componentName, "odh_metadata.yaml")) | ||
if err != nil { |
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 we need to distinguish between a file not present from an error, in this case we are swallowing it and I'm not sure that's the right approach as it could hide some other errors.
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.
this is done
ef0129c
to
4807565
Compare
4807565
to
13f55fd
Compare
<!---
Many thanks for submitting your Pull Request ❤️!
Please complete the following sections for a smooth review.
-->
Description
The PR is a first step to the feature initiative tracked in JIRA and this PR adds feature to capture RHOAI release versions from all the components(that are supported by DSC) repositories and include it in the DSC status.
JIRA: https://issues.redhat.com/browse/RHOAIENG-12619
How Has This Been Tested?
To test with DevFlags:
`
apiVersion: datasciencecluster.opendatahub.io/v1
kind: DataScienceCluster
metadata:
spec:
components:
status:
components:
datasciencepipelines
upstreamReleases:
- name: datasciencepipelines
displayName: Red Hat Build of Kubeflow Pipelines
version: 2.2.0
repoUrl: https://github.com/opendatahub-io/data-science-pipelines
`
Screenshot or short clip
Merge criteria