docs(example): interval requires count

[meeech]

docs(example): moved other count up next to interval for visibility

fixes: #2691

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
• The title of the PR is (a) conventional with a list of types and scopes found here, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
• I've signed my commits with DCO
• [ n/a ] 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.
• [ n/a ] My organization is added to USERS.md.

[github-actions]

Go Published Test Results

1 947 tests   1 947 ✔️  2m 35s ⏱️
   118 suites         0 💤
       1 files           0 ❌

Results for commit b8720b8.

♻️ This comment has been updated with latest results.

[codecov]

Codecov Report

Patch and project coverage have no change.

Comparison is base (b84e039) 81.47% compared to head (b8720b8) 81.47%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2690   +/-   ##
=======================================
  Coverage   81.47%   81.47%           
=======================================
  Files         133      133           
  Lines       20152    20152           
=======================================
  Hits        16419    16419           
  Misses       2881     2881           
  Partials      852      852           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[github-actions]

E2E Tests Published Test Results

    4 files      4 suites   3h 37m 42s ⏱️
  96 tests   82 ✔️   5 💤   9 ❌
402 runs  364 ✔️ 20 💤 18 ❌

For more details on these failures, see this check.

Results for commit b8720b8.

[agrawroh]

I don't think you need count when interval is specified. The docs clearly say:

"If both interval and count are omitted, the effective count is 1. If only interval is specified, metric runs indefinitely."

I would expect the analysis to keep running indefinitely in absence of the count.

[meeech]

@agrawroh 1.4.1
Does it make sense for it to run indefinitely when it's an inline step? Then the rollout would never complete? So I suppose the error makes sense in that we wouldnt want people to accidentally put themselves in a rollout that would never progress? Or would the only way out be a manual promote?

Could be the docs are a bit out of date - I could update the pr to update to the docs you pointed out - maybe we need them to be more specific - making the distinction between inline vs background analysis.

In any case, I would defer to the code over the docs - and assume docs need updating.

[agrawroh]

@agrawroh 1.4.1 Does it make sense for it to run indefinitely when it's an inline step? Then the rollout would never complete? So I suppose the error makes sense in that we wouldnt want people to accidentally put themselves in a rollout that would never progress? Or would the only way out be a manual promote?

Could be the docs are a bit out of date - I could update the pr to update to the docs you pointed out - maybe we need them to be more specific - making the distinction between inline vs background analysis.

In any case, I would defer to the code over the docs - and assume docs need updating.

You definitely need one of interval or count but I'm not convinced that we need to specify both. For example, if I just specify the interval with some success/failure counts, then I'd like the analysis to run and take as many measurements as possible and either fail or succeed based on the specified conditions. I can optionally specify the count to only take N measurements but it shouldn't be required if I have already specified the interval.
cc @harikrongali @zachaller

[meeech]

@agrawroh I see what you are saying. Could it be this is just a necessity for this particular type of analysis metric - Job when inline?
Or would Job with interval require a failureLimit and we add a new prop for successCount? For this Job metric type not sure how successCondition would work since its just exit code?

[zachaller]

The reason count is needed with interval here is because in the examples the way this template is used is in an inline step which has to have an end, otherwise the step would never complete. In background analysis you need interval but not count because it will just run till the rollout is done. I agree docs could probably be updated around this.

[zachaller]

This PR just fixes the case for how this template is used within the examples folder.