-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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: wrong Git file generator globbing (fixes #13284, #13313 and #13686) #13314
fix: wrong Git file generator globbing (fixes #13284, #13313 and #13686) #13314
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #13314 +/- ##
==========================================
+ Coverage 49.14% 49.16% +0.02%
==========================================
Files 248 248
Lines 42911 42944 +33
==========================================
+ Hits 21087 21115 +28
- Misses 19717 19718 +1
- Partials 2107 2111 +4
☔ View full report in Codecov by Sentry. |
cf1c5ba
to
9513b81
Compare
if I understand right, this would also fix #13284, right? |
Actually it won't. With this option it won't do any globbing at all, neither the wrong globbing it did before, nor correct globbing. But it would if I used the |
01ca256
to
e94541b
Compare
e94541b
to
c2ad69a
Compare
I have updated the PR to use the This should now also fix #13284. |
c2ad69a
to
ad4c8cf
Compare
ad4c8cf
to
03813f8
Compare
@ishitasequeira I have rebased this PR. Some of it had to be rewritten mostly because of #10952. It's now ready for review again. |
03813f8
to
ee3462a
Compare
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 @Calchan for the PR!! I tested the PR and was able to very the new globbing behavior with nested subcharts.
I went through the code and it looks good as well. Just a minor nit.
3b64105
to
2122bb0
Compare
Signed-off-by: Denis Dupeyron <denis.dupeyron@gmail.com>
2122bb0
to
51ffac1
Compare
I have fixed some rebase conflicts, so this PR is ready to merge again. |
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
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, @Calchan!
…rgoproj#13314) Signed-off-by: Denis Dupeyron <denis.dupeyron@gmail.com> Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
…rgoproj#13314) Signed-off-by: Denis Dupeyron <denis.dupeyron@gmail.com> Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
The current implementation of the Git file generator does undesirable globbing which breaks ApplicationSets in some cases. Please see full description of the issue in #13313.
I have extensively discussed this issue and PR with @crenshaw-dev on Slack at https://cloud-native.slack.com/archives/C01TSERG0KZ/p1681230308737899.
In this discussion, Micheal suggested I put the fix behind a configuration setting which defaults to the current behavior in case some users were relying on it. I did that, using the configuration for progressive syncs as a model.
Fixes: #13284
Fixes: #13313
Fixes: #13686
Checklist:
Please see Contribution FAQs if you have questions about your pull-request.