-
Notifications
You must be signed in to change notification settings - Fork 6.4k
feat: new jqPaths
ignore differences field for dynamic field selection (#24599)
#24658
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
base: master
Are you sure you want to change the base?
feat: new jqPaths
ignore differences field for dynamic field selection (#24599)
#24658
Conversation
…24599) - Add detection for multi-value jq expressions in ignoreApplicationDifferences - Implement jqMultiPathNormalizerPatch for handling annotation key selections - Fix issue where expressions like '.metadata.annotations | keys[] | select(startswith(...))' failed - Add comprehensive test coverage for multiple field scenarios Fixes argoproj#24599 Signed-off-by: puretension <[email protected]>
🔴 Preview Environment stopped on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
- Replace interface{} with any type for Go 1.18+ compatibility - Fix gofumpt formatting issues - Remove trailing spaces and extra blank lines Signed-off-by: puretension <[email protected]>
5fdaf7f
to
0711ef1
Compare
- Fix indentation and spacing issues - Remove extra blank lines - Align multi-line conditions properly Signed-off-by: puretension <[email protected]>
- Force CI to use latest formatted code - All gofumpt issues should be resolved Signed-off-by: puretension <[email protected]>
- Remove extra blank lines - Fix indentation in test file Signed-off-by: puretension <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #24658 +/- ##
==========================================
+ Coverage 60.46% 60.48% +0.01%
==========================================
Files 350 350
Lines 60105 60272 +167
==========================================
+ Hits 36345 36454 +109
- Misses 20828 20871 +43
- Partials 2932 2947 +15 ☔ View full report in Codecov by Sentry. |
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.
Thanks for the PR! Added a comment suggesting a different direction.
if pathStr, ok := v.(string); ok { | ||
pathsToDelete = append(pathsToDelete, pathStr) | ||
} |
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.
I think trying to infer the user's intent from the output of their expression is going to be too error-prone.
Consider this input:
{"a": 1, "b": "a", "c": [["a"]]}
Suppose we decide that, if the user's query evaluates to a string, we're going to treat it as a path.
If the user passes this query:
.b
then we'll receive the value "a"
, assume it's a path, and delete the "a"
key from the input. But that's unintuitive. The user obviously wanted to delete the "b"
key.
We could be more strict and say that the user must produce a valid delpaths()
input, i.e. a [][]string
.
But if the input object actually contains a [][]string
, we have no way of knowing whether the user is trying to pass us paths or has directly targeted something they want to delete:
.c
Output:
[["a"]]
Did the user mean to delete the .c
from the object, or .a
?
tl;dr, I don't think we should reuse the jqPathExpressions
field. I think we should create a new jqPaths
field. It should be an array of queries that produce jq paths:
each path is an array of strings and numbers
So the solution to the user's problem would be something like:
jqPaths:
- '.metadata?.annotations // {} | [keys | map(select(startswith("a")))]'
Then we should use delpaths()
instead of del()
to complete the mutation.
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.
@crenshaw-dev Thanks for the sharp analysis!
the examples with .b
and .c
really highlight how ambiguous and error-prone the current approach would be.
I completely agree with your reasoning. I’ll follow your suggestion and introduce a new jqPaths field (array of jq path queries), and update the implementation to use delpaths() instead of del().
I’ll update the schema, implementation, tests, and docs accordingly. 🙏
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.
I also added an item to the next Contributors' meeting to discuss w/ other maintainers. I'm always hesitant to add new features, so I want to make sure everyone feels its worth it.
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.
Thanks for bringing this to the Contributors' meeting!
I've implemented the jqPaths
approach you suggested.
Happy to push it for reference during the discussion! we can always adjust based on the team's feedback.
Looking forward to hearing everyone's thoughts! 🙏
Signed-off-by: puretension <[email protected]>
aff9682
to
310bf14
Compare
jqPaths
ignore differences field for dynamic field selection (#24599)
Summary
This PR fixes issue #24599 where
jqPathExpressions
inignoreApplicationDifferences
failed when expressions returned multiple values.Problem
The user's jq expression
.metadata.annotations? // {} | keys[] | select ( . | startswith("customprefix."))
returns multiple annotation keys, but ArgoCD wraps it withdel()
which cannot handle multiple values, causing a runtime error that gets silently ignored.Solution
jqMultiPathNormalizerPatch
for special handling of expressions that return multiple pathsChecklist: