-
Notifications
You must be signed in to change notification settings - Fork 6.6k
fix(applicationset): Git generator: Don't append default object to literal empty json array (#23500) #23513
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(applicationset): Git generator: Don't append default object to literal empty json array (#23500) #23513
Conversation
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
25d29bb to
eb40c3d
Compare
Signed-off-by: Christian Ciach <[email protected]>
eb40c3d to
7935218
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #23513 +/- ##
==========================================
- Coverage 60.25% 60.25% -0.01%
==========================================
Files 344 344
Lines 59084 59082 -2
==========================================
- Hits 35604 35597 -7
+ Misses 20611 20610 -1
- Partials 2869 2875 +6 ☔ View full report in Codecov by Sentry. |
rumstead
left a comment
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.
LGTM
nitishfy
left a comment
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.
LGTM!
…teral empty json array (argoproj#23500) (argoproj#23513) Signed-off-by: Christian Ciach <[email protected]>
…teral empty json array (argoproj#23500) (argoproj#23513) Signed-off-by: Christian Ciach <[email protected]> Signed-off-by: enneitex <[email protected]>
…teral empty json array (argoproj#23500) (argoproj#23513) Signed-off-by: Christian Ciach <[email protected]> Signed-off-by: Mangaal <[email protected]>
Checklist:
This is basically an improved version of #15661. That PR fixed the case where an empty yaml file wasn't distinguished from a missing file, which could easily lead to deleted applications when a user commented out all entries from their yaml file.
Unfortunately, that PR didn't distinguish between an empty yaml file from yaml files containing a literal empty json array (
[]) as the root element. Using arrays as the top-level structure for yaml files has long been supported by the git generator, but it seems to be an undocumented feature (or at least I couldn't find any documentation).If a yaml file contains an array as the top level element, the Git generator outputs a separate params-object for each element of the array. In other words, this feature can be used to easily template multiple applications from a single yaml file without the use of a matrix generator, as a user does this here:
Since this is a supported use-case, it stands to reason that an empty array (
[]) should result in an empty slice of objects returned by the git generator. But this has been broken since v2.10 due to #15661 failing to distinguish between an empty file (or{}) from an empty array, because the code previously tried to unmarshal the data into a slice, which even succeeds for empty files.To fix this I've just reversed the order of parsing attempts: This PR first tries to unmarshal the data into a
map, which even succeeds for empty files! This also makes the code a bit cleaner, since there is no need for constructing a dummy slice entry anymore. Only if parsing as a map fails (which is also the case for[]) we fall back to trying to unmarshal the data into a slice.I think it's also nice that the most common use case (yaml containing an object) is now tried first, while arrays are only attempted as a fallback. This slightly changed the returned error messsages, but no tests needed to be adjusted aside from that. I've added a unit test that fails with the previous implementation.
Kindly pinging @agaudreault because this builds on his previous PR.
Please see the comments of the linked issue for more details about the use-case and the cause of this issue.
Closes #23500