-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
docs: describe containerset retrystrategy and verify it works. Fixes: #11502 #12809
docs: describe containerset retrystrategy and verify it works. Fixes: #11502 #12809
Conversation
143bd5a
to
1752752
Compare
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 add a note here
- https://github.com/argoproj/argo-workflows/blob/v3.5.5/pkg/apis/workflow/v1alpha1/container_set_template_types.go#L16
- https://github.com/argoproj/argo-workflows/blob/v3.5.5/docs/container-set-template.md
so that user can understand the behaviour from docs instead of going through the issue and PR
Yes, it needs to be described more clearly in the comments and documentation. I will add more later. 🙏 |
1752752
to
4ca5dc2
Compare
4ca5dc2
to
465c5ec
Compare
Signed-off-by: shuangkun <[email protected]>
0a7fe4c
to
7ef20e6
Compare
Signed-off-by: shuangkun <[email protected]>
7ef20e6
to
e57439f
Compare
Signed-off-by: shuangkun <[email protected]>
6bdf3fe
to
157b0fd
Compare
Signed-off-by: shuangkun <[email protected]>
157b0fd
to
609b565
Compare
Signed-off-by: shuangkun <[email protected]>
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 should've caught this earlier, I think the test should be somewhere else instead of cli
.
Imagine in CI when there's failure in cli test
but totally unrelated to cli
, this doesn't sound right.
What do you think?
#12864 has not been implemented yet, so that won't work yet.
As I said in my previous comment, I will retry CI myself when this is ready to merge. No need to spend time getting E2Es to pass in the mean-time. Just need to make sure CI passes for docs + codegen for each change. |
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.
A few final changes here to account for the differences between a template-level retryStrategy
and the inner containerSet.retryStrategy
as discussed in a previous in-line comment
Co-authored-by: Anton Gilgur <[email protected]> Signed-off-by: shuangkun tian <[email protected]>
Co-authored-by: Anton Gilgur <[email protected]> Signed-off-by: shuangkun tian <[email protected]>
Co-authored-by: Anton Gilgur <[email protected]> Signed-off-by: shuangkun tian <[email protected]>
Co-authored-by: Anton Gilgur <[email protected]> Signed-off-by: shuangkun tian <[email protected]>
Signed-off-by: shuangkun <[email protected]>
Signed-off-by: shuangkun <[email protected]>
Signed-off-by: shuangkun <[email protected]>
e503aae
to
87ac23d
Compare
Signed-off-by: shuangkun <[email protected]>
Signed-off-by: shuangkun <[email protected]>
Signed-off-by: shuangkun <[email protected]>
Signed-off-by: shuangkun <[email protected]>
Signed-off-by: shuangkun <[email protected]>
I modified it and fixed errors. Thanks! |
Signed-off-by: Anton Gilgur <[email protected]>
I pushed a commit myself to fix the |
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.
Thanks for adding these docs and iterating on this!
OK,Thanks! |
…11502 (#12809) Signed-off-by: shuangkun <[email protected]> Signed-off-by: shuangkun tian <[email protected]> Signed-off-by: Anton Gilgur <[email protected]> Co-authored-by: Anton Gilgur <[email protected]> Co-authored-by: Anton Gilgur <[email protected]> (cherry picked from commit 459b09d)
Fixes #11502
Motivation
Show how containerset retrystrategy works.
Modifications
Verification
e2e test