search

open-cluster-management-io / api

Core APIs for open cluster management
https://open-cluster-management.io
Apache License 2.0
251 stars 77 forks source link

:sparkles: Replace `Timeout` with `RolloutConfig` #281

Closed dhaiducek closed 1 year ago

dhaiducek commented 1 year ago

Summary

Implement RolloutConfig, which provides more fine-tuned configuration for users than the current Timeout.

Related issue(s)

qiujian16 commented 1 year ago

@dhaiducek since we are about to release 0.12.0, would like to know the progress of this PR. Is it ready for review, do you plan to include it in 0.12.0 or it can be moved to next release?

dhaiducek commented 1 year ago

@qiujian16 It was waiting on #276, but now that it's merged I'll continue work here. I'll work on it this week, but I think it's also fine if v0.12.0 is released without these library updates so that this PR doesn't block the release.

dhaiducek commented 1 year ago

Status update: I wrestled with the CI a bit because I'm on an M1 MacBook and some of the binaries aren't actually built for arm64 (those changes are in this PR). I've got current code working, migrating Timeout --> ProgressDeadline, but need to add logic (and tests) for the other fields that are in the new struct alongside it.

dhaiducek commented 1 year ago

/cc @serngawy @haoqing0110

Sorry for the long delay here--between wrestling with the CI and getting pulled away for other tasks, it's taken longer to update than I thought. There might be some more tests required here, but I think the logic is largely (if not entirely) there.

Some notes:

serngawy commented 1 year ago

Sounds good to me in general, I added more unit test to cover Rollout ProgressivePerGroup case.

dhaiducek commented 1 year ago

After discussion with @serngawy I've updated the logic so that MandatoryDecisionGroups tolerate no failures for any progressive rollout type.

haoqing0110 commented 1 year ago

LGTM, seems the ProgressDeadline is just a rename of Timeout, I'm fine to leave the Timeout and mark it as deprecated.

qiujian16 commented 1 year ago

/approve

Since manifestworkReplicaSet and ClusterManagerAddon install strategy is in alpha stage. Let's remove timeout in the next release (ie 0.14.0)

dhaiducek commented 1 year ago

Since manifestworkReplicaSet and ClusterManagerAddon install strategy is in alpha stage. Let's remove timeout in the next release (ie 0.14.0)

Sounds good, thanks @qiujian16!

@serngawy @haoqing0110 I'd appreciate if you'd do another review here when you have a moment! 😄

serngawy commented 1 year ago

Since manifestworkReplicaSet and ClusterManagerAddon install strategy is in alpha stage. Let's remove timeout in the next release (ie 0.14.0)

Sounds good, thanks @qiujian16!

@serngawy @haoqing0110 I'd appreciate if you'd do another review here when you have a moment! 😄

I gave it another review, sounds good to me @dhaiducek appreciate the good work :)

ops, didn't consider my approve will merge it @haoqing0110

openshift-ci[bot] commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhaiducek, qiujian16, serngawy

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: - ~~[OWNERS](https://github.com/open-cluster-management-io/api/blob/main/OWNERS)~~ [qiujian16] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment