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: metric fields can be parameterized by analysis arguments #901

Merged
merged 30 commits into from
Jan 8, 2021

Conversation

khhirani
Copy link
Contributor

Closes #825

Signed-off-by: khhirani [email protected]

@khhirani khhirani requested a review from jessesuen December 15, 2020 03:00
@codecov-io
Copy link

codecov-io commented Dec 15, 2020

Codecov Report

Attention: Patch coverage is 78.66667% with 16 lines in your changes missing coverage. Please review.

Project coverage is 81.60%. Comparing base (2d90163) to head (8953e8c).
Report is 1207 commits behind head on master.

Files with missing lines Patch % Lines
utils/analysis/factory.go 73.68% 5 Missing and 5 partials ⚠️
.../apis/rollouts/validation/validation_references.go 55.55% 2 Missing and 2 partials ⚠️
analysis/analysis.go 92.59% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #901      +/-   ##
==========================================
- Coverage   81.74%   81.60%   -0.15%     
==========================================
  Files          99       99              
  Lines        8684     8732      +48     
==========================================
+ Hits         7099     7126      +27     
- Misses       1138     1151      +13     
- Partials      447      455       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

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

I see you made changes to all of the types, but I don't see any tests which actually test the feature. Where is a test which actually does substitution for a metric like:

  metrics:
  - name: error-rate
    provider:
      prometheus:
        interval: "{{args.interval}}"
        count: "{{args.count}}"

pkg/apis/rollouts/v1alpha1/analysis_types.go Outdated Show resolved Hide resolved
@jessesuen
Copy link
Member

Also, an e2e would be nice, even if it is just modifying an existing one.

@khhirani khhirani force-pushed the configurable-analysis branch from 0f1bebc to bcca948 Compare December 15, 2020 22:10
Signed-off-by: khhirani <[email protected]>
Signed-off-by: khhirani <[email protected]>
Signed-off-by: khhirani <[email protected]>
@khhirani khhirani requested a review from jessesuen December 16, 2020 05:33
@jessesuen jessesuen changed the title feat: Configurable AnalysisTemplate feat: metric fields can be parameterized by analysis arguments Dec 17, 2020
@jessesuen
Copy link
Member

jessesuen commented Dec 17, 2020

@khhirani you'll need to rebase on master to fix the codegen problem. I found a toolchain discrepancy between macOS kubectl vs. github's ubuntu-latest kubectl that causes the difference in manifests. I switched to using kustomize build instead of kubectl kustomize which fixed the problem, assuming you are running a moderately recent version of kustomize. Github runs v3.8

Signed-off-by: khhirani <[email protected]>
Signed-off-by: khhirani <[email protected]>
Signed-off-by: khhirani <[email protected]>
Signed-off-by: khhirani <[email protected]>
Signed-off-by: khhirani <[email protected]>
Signed-off-by: khhirani <[email protected]>
@khhirani
Copy link
Contributor Author

@jessesuen Fixed

@jessesuen jessesuen self-assigned this Dec 17, 2020
Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

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

I see that everywhere where count used to be nil, is now doing count := intstr.FromInt(0) and I'd like to understand why, since nil and zero mean different things.

docs/features/bluegreen.md Outdated Show resolved Hide resolved
utils/defaults/defaults.go Outdated Show resolved Hide resolved
rollout/analysis_test.go Outdated Show resolved Hide resolved
analysis/analysis_test.go Show resolved Hide resolved
analysis/analysis_test.go Outdated Show resolved Hide resolved
Signed-off-by: khhirani <[email protected]>
Signed-off-by: khhirani <[email protected]>
@khhirani khhirani requested a review from jessesuen January 6, 2021 00:00
@@ -57,7 +57,7 @@ spec:
strategy:
blueGreen:
autoPromotionEnabled: boolean
autoPromotionSeconds: *int32
autoPromotionSeconds: intOrString
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you changed this to intOrString

Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

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

LGTM, please revert unintentional change to bluegreen.md before merging

@sonarcloud
Copy link

sonarcloud bot commented Jan 8, 2021

Kudos, SonarCloud Quality Gate passed!

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

No Coverage information No Coverage information
4.2% 4.2% Duplication

@khhirani khhirani merged commit 1beffd4 into argoproj:master Jan 8, 2021
@khhirani khhirani deleted the configurable-analysis branch January 8, 2021 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make AnalysisTemplate more configurable
3 participants