feat(analysis): Allow analysis arguments to get valueFrom Rollout status (#1242) #1629

noam-codefresh · 2021-11-08T13:31:40Z

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.

codecov · 2021-11-08T13:40:29Z

Codecov Report

Merging #1629 (b35fcb5) into master (6edc9c8) will decrease coverage by 0.00%.
The diff coverage is 83.63%.

@@            Coverage Diff             @@
##           master    #1629      +/-   ##
==========================================
- Coverage   81.99%   81.98%   -0.01%     
==========================================
  Files         115      116       +1     
  Lines       15913    15974      +61     
==========================================
+ Hits        13048    13097      +49     
- Misses       2196     2204       +8     
- Partials      669      673       +4

Impacted Files	Coverage Δ
rollout/analysis.go	`78.90% <25.00%> (-0.70%)`	⬇️
rollout/experiment.go	`82.69% <25.00%> (-1.29%)`	⬇️
utils/analysis/factory.go	`94.02% <93.18%> (-0.42%)`	⬇️
pkg/apis/rollouts/validation/validation.go	`94.06% <100.00%> (ø)`
.../apis/rollouts/validation/validation_references.go	`85.19% <100.00%> (+0.05%)`	⬆️
rollout/trafficrouting/istio/controller.go	`50.81% <0.00%> (-1.63%)`	⬇️
controller/metrics/metrics.go	`100.00% <0.00%> (ø)`
utils/conditions/conditions.go	`80.76% <0.00%> (ø)`
controller/metrics/prommetrics.go	`100.00% <0.00%> (ø)`
... and 1 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 6edc9c8...b35fcb5. Read the comment docs.

jessesuen

Could you provide documentation on this? It's not clear how this will work and how to use it.

jessesuen · 2021-11-08T20:34:24Z

go.mod

@@ -3,6 +3,7 @@ module github.com/argoproj/argo-rollouts
 go 1.16

 require (
+	github.com/PaesslerAG/jsonpath v0.1.1


Hmmm, this library uses "BSD 3-Clause "New" or "Revised" License" which is not what we typically use (Apache 2 or MIT). I'm not sure this is okay to use. Is there something else we can use or write our own?

I tried using a different library called objx, which was similar. But the problem there was that using a non-existing path (path: "some.none.existing[4].path.in.rollout") just returns an empty string. So it's not possible to display an error when the user used an invalid path or not (unless we assume an empty string is always an error).

If you know of any other possible libraries, I'd be happy to give them a shot as well.
I can also write a simple parser by splitting on ".". I don't think it will be too complicated, given our very specific use-case (specifically with field names that do not contain dots themselves)

noam-codefresh · 2021-11-14T13:39:12Z

I removed the dependency on jsonPath, and now the code is parsing the path directly. It will support dot notation and array indexers, so something like status.canary.weights.canary.podTemplateHash (the original request), or status.conditions[0].status, but not anything more complex

harikrongali · 2021-11-16T01:40:58Z

utils/analysis/factory.go

+	var isArray, isMap bool
+	_, isArray = m.([]interface{})
+	_, isMap = m.(map[string]interface{})
+	if isArray || isMap {


what is this check is for? can you provide an example?

If the user puts in something like status.canary.weights.canary - the resulting value would be a map, and not a primitive value (string/number/bool).
I guess we could support that as well, and just return it as a JSON string, but is it a reasonable usecase? It seems like it might be a future ticket.

harikrongali · 2021-11-16T01:41:42Z

utils/analysis/factory.go

@@ -217,3 +227,42 @@ func ValidateMetric(metric v1alpha1.Metric) error {
 	}
 	return nil
 }
+
+func extractValueFromRollout(r *v1alpha1.Rollout, path string) (string, error) {


can you add negative tests if possible as coverage is red for following chunks

I added some tests for invalid paths.
I also removed the error check after the json.Marshal - I assume that a valid Rollout object can always be marshalled to json.

Signed-off-by: Noam Gal <[email protected]>

return err if path is inavlid in runtime (don't check in validation time) Signed-off-by: Noam Gal <[email protected]>

Signed-off-by: Noam Gal <[email protected]>

harikrongali · 2021-11-18T22:09:03Z

go.mod

@@ -32,6 +32,7 @@ require (
 	github.com/soheilhy/cmux v0.1.4
 	github.com/spaceapegames/go-wavefront v1.8.1
 	github.com/spf13/cobra v1.1.3
+	github.com/stretchr/objx v0.3.0 // indirect


are we using this library?

you are right. fixing now.

harikrongali · 2021-11-18T22:20:48Z

LGTM..except the comment about lib objx

Signed-off-by: Noam Gal <[email protected]>

sonarcloud · 2021-11-21T12:54:53Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

No Coverage information
13.6% Duplication

magec · 2021-12-21T13:34:05Z

This is exactly what I am looking for, I need to make prometheus queries using status.currentPodHash, my life would be way easier with this feature. Looking forward for the merge.

Thanks @noam-codefresh!

alexmt · 2021-12-21T17:25:29Z

@jessesuen can you take another look please ?

jessesuen

Minor nit about documentation but otherwise LGTM

jessesuen · 2021-12-21T22:50:02Z

docs/features/analysis.md

+Analysis arguments also support valueFrom for reading any Rollout fields and passing them as arguments to AnalysisTemplate.
+An example would be to reference metadata labels like env and region and passing them along to AnalysisTemplate, or any field
+from the Rollout status


Since we don't have versioned docs yet, could we add a note stating v1.2 supports valueFrom rollout status?

magec · 2022-01-12T08:45:29Z

Can this be merged? I have run E2E tests locally and they seem to work well.

noam-codefresh mentioned this pull request Nov 8, 2021

Allow analysis arguments to get valueFrom Rollout status #1242

Closed

jessesuen requested changes Nov 8, 2021

View reviewed changes

noam-codefresh force-pushed the value_from_status branch from 642a571 to d91a9ed Compare November 15, 2021 17:48

harikrongali reviewed Nov 16, 2021

View reviewed changes

ATGardner added 11 commits November 17, 2021 17:53

use objx to read value from Rollout manifest

9fd2b58

Signed-off-by: Noam Gal <[email protected]>

handle [] annotation correcly in BuildArgumentsForRolloutAnalysisRun

f0e3ec2

Signed-off-by: Noam Gal <[email protected]>

validate valueFrom correctly

d7a69c2

Signed-off-by: Noam Gal <[email protected]>

use jsonpath instead of objx

6c75f7c

return err if path is inavlid in runtime (don't check in validation time) Signed-off-by: Noam Gal <[email protected]>

parse path in code, instead of using jsonPath

e9a0572

Signed-off-by: Noam Gal <[email protected]>

fixed test

d213140

Signed-off-by: Noam Gal <[email protected]>

updated documentation

8573e22

Signed-off-by: Noam Gal <[email protected]>

added tests for coverage

7267c63

Signed-off-by: Noam Gal <[email protected]>

fixed lint

e56cdb6

Signed-off-by: Noam Gal <[email protected]>

added another test case

63d3369

Signed-off-by: Noam Gal <[email protected]>

fixed case when path ends with "]"

1eab4fd

Signed-off-by: Noam Gal <[email protected]>

noam-codefresh force-pushed the value_from_status branch from 5819e64 to 1eab4fd Compare November 17, 2021 15:53

harikrongali reviewed Nov 18, 2021

View reviewed changes

removed objx dependency

b35fcb5

Signed-off-by: Noam Gal <[email protected]>

harikrongali approved these changes Dec 13, 2021

View reviewed changes

alexmt approved these changes Dec 13, 2021

View reviewed changes

jessesuen approved these changes Dec 21, 2021

View reviewed changes

alexmt merged commit 4739bcd into argoproj:master Jan 12, 2022

harikrongali mentioned this pull request Jan 12, 2022

docs: fix doc for valueFrom fields for analysis args #1763

Merged

6 tasks

noam-codefresh deleted the value_from_status branch February 9, 2022 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(analysis): Allow analysis arguments to get valueFrom Rollout status (#1242) #1629

feat(analysis): Allow analysis arguments to get valueFrom Rollout status (#1242) #1629

noam-codefresh commented Nov 8, 2021

codecov bot commented Nov 8, 2021 •

edited

Loading

jessesuen left a comment

jessesuen Nov 8, 2021

noam-codefresh Nov 10, 2021

noam-codefresh commented Nov 14, 2021

harikrongali Nov 16, 2021

noam-codefresh Nov 17, 2021

harikrongali Nov 16, 2021

noam-codefresh Nov 17, 2021

harikrongali Nov 18, 2021

noam-codefresh Nov 21, 2021

harikrongali commented Nov 18, 2021

sonarcloud bot commented Nov 21, 2021

magec commented Dec 21, 2021

alexmt commented Dec 21, 2021

jessesuen left a comment

jessesuen Dec 21, 2021

magec commented Jan 12, 2022

feat(analysis): Allow analysis arguments to get valueFrom Rollout status (#1242) #1629

feat(analysis): Allow analysis arguments to get valueFrom Rollout status (#1242) #1629

Conversation

noam-codefresh commented Nov 8, 2021

codecov bot commented Nov 8, 2021 • edited Loading

Codecov Report

jessesuen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noam-codefresh commented Nov 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harikrongali commented Nov 18, 2021

sonarcloud bot commented Nov 21, 2021

magec commented Dec 21, 2021

alexmt commented Dec 21, 2021

jessesuen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magec commented Jan 12, 2022

codecov bot commented Nov 8, 2021 •

edited

Loading