fix(experiments): fire rollout event on experiment step #4124
Conversation
Signed-off-by: mitchell amihod <[email protected]>
… a Rollout. We can then identify when an experiment is a Step in a Rollout. Signed-off-by: mitchell amihod <[email protected]>
meeech
force-pushed
the
4009-fire-rollout-event-on-experiment-step
branch
from
f8634b0 to
64f8285
Compare
Published E2E Test Results 4 files 4 suites 3h 9m 58s ⏱️ For more details on these failures, see this check. Results for commit bd4aa34. ♻️ This comment has been updated with latest results. |
Published Unit Test Results2 300 tests 2 300 ✅ 2m 59s ⏱️ Results for commit bd4aa34. ♻️ This comment has been updated with latest results. |
meeech
force-pushed
the
4009-fire-rollout-event-on-experiment-step
branch
from
31f99c3 to
7aa0804
Compare
|
@zachaller tightened things up a bit. |
Signed-off-by: mitchell amihod <[email protected]>
…he Experiment Addresses argoproj#4009. This change will fire Analysis Run events bound to the parent Rollout object when the Experiment is a Step in the Rollout. Signed-off-by: mitchell amihod <[email protected]>
If we pass belongs to rollout check, we know there is a rollout to find. Signed-off-by: mitchell amihod <[email protected]>
meeech
force-pushed
the
4009-fire-rollout-event-on-experiment-step
branch
from
7aa0804 to
2c4dfa0
Compare
Signed-off-by: mitchell amihod <[email protected]>
meeech
force-pushed
the
4009-fire-rollout-event-on-experiment-step
branch
from
2c4dfa0 to
bd4aa34
Compare
|
| // #4009 | ||
| roRef := experimentutil.GetRolloutOwnerRef(ec.ex) | ||
| if roRef != nil { | ||
| rollout, err := ec.argoProjClientset.ArgoprojV1alpha1().Rollouts(ec.ex.Namespace).Get(context.TODO(), roRef.Name, metav1.GetOptions{}) |
There was a problem hiding this comment.
I am ok leaving this as a client call because the rest of the experiment code does this but it would be nice if we had access to the rollouts informer in experiment controller
There was a problem hiding this comment.
i'd be open to making that change (maybe later iteration - i think i will be back in this code later around fixing the complation/delay bug I reported). I wasn't sure about the implications around a changelike that, so I took a light touch.
This fix (feat?) resolves the issue raised in #4009.
I think this meets what users would expect from the use of an Experiment as a Step in a Rollout.
As a user, if I setup notifications on AnalysisRun events, I would expect an AnalysisRun that fails should generate a notification. To be clear, this is ONLY when the Analysis Run belongs to an Experiment, which itself belongs to the Rollout (As a Step in that Rollout). https://argo-rollouts.readthedocs.io/en/stable/features/experiment/#integration-with-rollouts
Please let me know if you would like to discuss this further, but this feels like a 'safe' change. It should even be good to backport to 1.7x (which is what I developed it against).
I could use some guidance around testing this - I didn't see any prior integration test for this code path. But maybe there's some e2e and I wasn't having success finding it.
Checklist:
"fix(controller): Updates such and such. Fixes #1234".