Skip to content
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: add local variable pod.retryAttempt. Fixes #2808, Fixes #2898. #2911

Merged
merged 5 commits into from
May 8, 2020

Conversation

seddonm1
Copy link
Contributor

@seddonm1 seddonm1 commented Apr 30, 2020

  • add pod.retryAttempt that reports the retry attempt number starting at 0 for first attempt and increasing by 1 each attempt.
  • only executes on nodes with retryStrategy defined.
  • updated the examples/retry-with-steps.yaml to demonstrate usage.
  • updated the docs/variables.md to describe variable.

Fixes #2808 and Fixes #2898 for enhancement request.

Checklist:

  • Either (a) I've created an [enhancement proposal]
  • The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I've signed the CLA.
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md. -- N/A

@seddonm1 seddonm1 changed the title (feat) add local variable pod.retryAttempt (see #2808, #2898) feat: add local variable pod.retryAttempt (see #2808, #2898) Apr 30, 2020
@simster7 simster7 self-assigned this May 1, 2020
@simster7 simster7 self-requested a review May 1, 2020 05:13
Copy link
Member

@simster7 simster7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great start! Some comments to fix

docs/variables.md Outdated Show resolved Hide resolved
workflow/controller/operator.go Outdated Show resolved Hide resolved
workflow/controller/operator.go Outdated Show resolved Hide resolved
workflow/controller/operator.go Outdated Show resolved Hide resolved
workflow/controller/operator.go Show resolved Hide resolved
workflow/common/common.go Outdated Show resolved Hide resolved
Copy link
Member

@simster7 simster7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking better!

workflow/common/common.go Outdated Show resolved Hide resolved
workflow/controller/operator.go Show resolved Hide resolved
workflow/controller/operator.go Outdated Show resolved Hide resolved
workflow/controller/operator.go Show resolved Hide resolved
@simster7
Copy link
Member

simster7 commented May 2, 2020

This LGTM as is, but I want to confirm with the rest of the team that retryAttempt is the name we want for this (as opposed to something like retryNumber or attemptNumber or the like) before approving and merging.

@seddonm1
Copy link
Contributor Author

seddonm1 commented May 2, 2020

Great. Thanks Simon. Naming is half the battle.

@simster7
Copy link
Member

simster7 commented May 6, 2020

Hey @seddonm1, sorry for the delay. We decided to name this flag {{retries}}. The idea is that when you run your first attempt, it is technically attempt number 1, but {{retryAttempts}} would say 0. This might be a bit of a misnomer. {{retries}} however indicates that there have been 0 retries, which is a tad more accurate.

Can you update the naming and resolve the conflicts please?

@seddonm1 seddonm1 requested a review from jessesuen as a code owner May 6, 2020 22:50
@seddonm1
Copy link
Contributor Author

seddonm1 commented May 6, 2020

this code is now up to date and has the variable named retries.

@simster7
Copy link
Member

simster7 commented May 7, 2020

@seddonm1 Seems like there was a Git issue when you rebased/merged with master. You can see that this PR now changes 150+ files, which is not desired.

Can you solve this and make sure that it only contains your desired changes?

@seddonm1 seddonm1 force-pushed the add-pod-retryAttempt branch from 376bbaf to e0de88d Compare May 8, 2020 00:01
@sonarqubecloud

This comment was marked as resolved.

@seddonm1
Copy link
Contributor Author

seddonm1 commented May 8, 2020

@simster7 Sorry. not sure what happened with my client. fixed.

@simster7 simster7 merged commit d8cb66e into argoproj:master May 8, 2020
@agilgur5 agilgur5 changed the title feat: add local variable pod.retryAttempt (see #2808, #2898) feat: add local variable pod.retryAttempt. Fixes #2808, Fixes #2898. Oct 18, 2024
@agilgur5 agilgur5 added area/templating Templating with `{{...}}` area/retryStrategy Template-level retryStrategy labels Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/retryStrategy Template-level retryStrategy area/templating Templating with `{{...}}`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants