update: enable release component info for feast #1625
Conversation
| @@ -42,6 +43,7 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl. | |||
| // Add FeastOperator-specific actions | |||
| WithAction(initialize). | |||
| WithAction(devFlags). | |||
| WithAction(releases.NewAction()). | |||
There was a problem hiding this comment.
Probably, it should also implement WithReleases interface.
Thoughts: This action is common for all the components. I'm wondering would it be possible somehow to make a more common wrapper. It mus be called after manifests configured from the devFlags. Another question, if it should be done like that. I mean, may be release information should not be substituted with devFlags.
There was a problem hiding this comment.
two parts from me:
- we can move the release releated into something as you suggested WithRelease()
originally, i see it is close to updatestatus but even more close to the way we set the release info from DSC/DSCI which is a one time action (this goes away after point (2) comes in)
in short: we can refactor later. for now, it is functional as expected. - i had thoughts on this order as well. i convinced myself eventually, it needs to be done after devFlags. the reason is, if component want to test different value by having a mock value in the yaml file, so they can easily know the one running in the cluster is a mock/dev manifests not the original one from the operator build.
and with some ongoing support multi-arch work, that will be even helpful for the test purpose.
There was a problem hiding this comment.
- we can move the release releated into something as you suggested WithRelease()
I mean, the action requires Feast to implement the interface https://github.com/opendatahub-io/opendatahub-operator/blob/main/pkg/controller/actions/status/releases/action_fetch_releases_status.go#L63
There was a problem hiding this comment.
- for now, it is functional as expected.
wondering how. Did I look wrong code?
There was a problem hiding this comment.
i missed this part, thought i added it in the push.........................
There was a problem hiding this comment.
@zdtsw you are also missing the updated generated files.
|
/retest |
This PR can't be merged just yet 😢Please run For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/13198314525 |
- test is disabled since yaml file is not sync to ODH feast repo yet Signed-off-by: Wen Zhou <[email protected]>
Signed-off-by: Wen Zhou <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ugiordan, ykaliuta The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test opendatahub-operator-e2e |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1625 +/- ##
==========================================
- Coverage 20.28% 20.28% -0.01%
==========================================
Files 163 163
Lines 11137 11138 +1
==========================================
Hits 2259 2259
- Misses 8638 8639 +1
Partials 240 240 ☔ View full report in Codecov by Sentry. |
|
update the yaml file for |
|
New changes are detected. LGTM label has been removed. |
|
/test opendatahub-operator-e2e |
|
@zdtsw: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Description
https://issues.redhat.com/browse/RHOAIENG-19100
https://issues.redhat.com/browse/RHOAIENG-11648
How Has This Been Tested?
Screenshot or short clip
Merge criteria