-
Notifications
You must be signed in to change notification settings - Fork 793
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
fix: allow namespace default using valueFrom as arg for AnalysisTempl… #3436
base: master
Are you sure you want to change the base?
fix: allow namespace default using valueFrom as arg for AnalysisTempl… #3436
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3436 +/- ##
==========================================
+ Coverage 81.83% 82.88% +1.04%
==========================================
Files 135 135
Lines 20688 17158 -3530
==========================================
- Hits 16931 14221 -2710
+ Misses 2883 2044 -839
- Partials 874 893 +19 ☔ View full report in Codecov by Sentry. |
Go Published Test Results2 161 tests 2 161 ✅ 2m 53s ⏱️ Results for commit 758eada. ♻️ This comment has been updated with latest results. |
E2E Tests Published Test Results 6 files 6 suites 5h 21m 4s ⏱️ For more details on these failures, see this check. Results for commit 758eada. ♻️ This comment has been updated with latest results. |
dd458af
to
51aab55
Compare
func resolveArgsFromValueFromFieldRef(arg v1alpha1.Argument, namespace string) v1alpha1.Argument { | ||
if arg.ValueFrom.FieldRef != nil && arg.ValueFrom.FieldRef.FieldPath != "" { | ||
switch arg.ValueFrom.FieldRef.FieldPath { | ||
case "metadata.namespace": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I am following this correctly does this mean that only metadata.namespace works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering why this does not seem be called form this path:
argo-rollouts/utils/analysis/factory.go
Line 34 in c6ba82c
value, err = fieldpath.ExtractFieldPathAsString(r, arg.ValueFrom.FieldRef.FieldPath) |
How are you creating the AnalysisRun, is the argo rollouts controller creating it or are you creating it say via kubectl or something similar outside of rollouts controller? |
So it seems like the problem here is mainly that when wanting to not have to pass in the namespace from the Rollout to the AnalysisTemplate. If the arg is not specified but is referenced in the AnalysisTemplate like namespace, it should default to the namespace from the Rolllout to be used in metrics provider like Prometheus. Thanks @zachaller , I will take a deeper look into the ExtractFieldPathAsString to see if there is a good way to handle this kind of default. |
…ate (argoproj#3361) Signed-off-by: Craig Newton <newtondev@gmail.com>
51aab55
to
758eada
Compare
Quality Gate passedIssues Measures |
…ate (#3361)
Checklist:
"fix(controller): Updates such and such. Fixes #1234"
.