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.

Sign up for GitHub

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

Jump to bottom

docs: fix some vague description about analysis arguments #1672

Merged
merged 1 commit into from
Dec 14, 2021
Merged

docs: fix some vague description about analysis arguments #1672

merged 1 commit into from
Dec 14, 2021

Conversation

huikang
Copy link
Member

@huikang huikang commented Nov 26, 2021

Signed-off-by: Hui Kang hui.kang@salesforce.com

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, (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
  • 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.

@huikang huikang force-pushed the doc-fix-analysis-args branch from 3d9a737 to 78bed3e Compare November 26, 2021 05:40
@codecov
Copy link

codecov bot commented Nov 26, 2021 •
edited
Loading

Codecov Report

Merging #1672 (4108196) into master (b3d50d1) will decrease coverage by 0.07%.
The diff coverage is n/a.

❗ Current head 4108196 differs from pull request most recent head 4d9cec0. Consider uploading reports for the commit 4d9cec0 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1672      +/-   ##
==========================================
- Coverage   82.06%   81.99%   -0.08%     
==========================================
  Files         116      116              
  Lines       16075    15929     -146     
==========================================
- Hits        13192    13061     -131     
+ Misses       2210     2198      -12     
+ Partials      673      670       -3
Impacted Files Coverage Δ
utils/analysis/helpers.go 76.99% <0.00%> (-1.19%) ⬇️
analysis/analysis.go 84.74% <0.00%> (-1.00%) ⬇️
rollout/temlateref.go 82.16% <0.00%> (-0.83%) ⬇️
controller/metrics/analysis.go 87.50% <0.00%> (-0.18%) ⬇️
rollout/controller.go 74.71% <0.00%> (-0.10%) ⬇️
rollout/pause.go 95.33% <0.00%> (ø)
rollout/restart.go 98.64% <0.00%> (ø)
rollout/replicaset.go 67.59% <0.00%> (ø)
analysis/controller.go 52.17% <0.00%> (ø)
utils/metric/metric.go 100.00% <0.00%> (ø)
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3d50d1...4d9cec0. Read the comment docs.

@huikang huikang force-pushed the doc-fix-analysis-args branch from 78bed3e to 6c50e00 Compare November 26, 2021 21:48
agrawroh
agrawroh reviewed Dec 2, 2021
View reviewed changes
docs/features/analysis.md
@@ -356,7 +356,7 @@ templates together. The controller combines the `metrics` and `args` fields of a
The controller will error when merging the templates if:

* Multiple metrics in the templates have the same name
* Two arguments with the same name both have values
* Two arguments with the same name have different default values no matter the argument value in Rollout
Copy link
Member

Choose a reason for hiding this comment

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

I failed to understand this completely. Is this more clear by any chance?

"Two arguments with the same name have different default values. It doesn't matter what the argument values are in the Rollout."

Copy link
Member Author

Choose a reason for hiding this comment

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

The original text is wrong in that if Two arguments have different default values, the merging is valid. So the updated text adds the additional condition of have different default values

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Thanks for explaining :)

agrawroh
agrawroh approved these changes Dec 2, 2021
View reviewed changes
Copy link
Member

@agrawroh agrawroh left a comment

Choose a reason for hiding this comment

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

                 —— SHIP IT ——

                   ,:',:`,:'
                __||_||_||_||__
           ____["""""""""""""""]____
           \ " '''''''''''''''''''' |
    ~^~~^~^~^^~^~^~^~^~^~^~^~~^~^~^^~~^~^

@huikang huikang force-pushed the doc-fix-analysis-args branch from 4108196 to 0646576 Compare December 9, 2021 02:37
docs: fix some vague description about analysis arguments
4d9cec0 
- multiple templates in rollouts

Signed-off-by: Hui Kang <hui.kang@salesforce.com>
@huikang huikang force-pushed the doc-fix-analysis-args branch from 0646576 to 4d9cec0 Compare December 13, 2021 03:56
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
3.2% 3.2% Duplication

@jessesuen jessesuen merged commit e243276 into argoproj:master Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agrawroh agrawroh agrawroh approved these changes

@jessesuen jessesuen jessesuen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@huikang @agrawroh @jessesuen