fix(cli): uses DrySource revision for app diff/manifests with sourceHydrator (#23817) #24670
Conversation
✅ Preview Environment deployed on Bunnyshell
See: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
Signed-off-by: Aditya Raj <[email protected]>
adityaraj178
force-pushed
the
diff-manifest-cmd-sourceHydrator
branch
from
1e60695 to
821e0d5
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #24670 +/- ##
==========================================
+ Coverage 60.46% 60.52% +0.06%
==========================================
Files 350 350
Lines 60187 60195 +8
==========================================
+ Hits 36390 36431 +41
+ Misses 20859 20833 -26
+ Partials 2938 2931 -7 ☔ View full report in Codecov by Sentry. |
server/application/application.go
Outdated
| if q.GetRevision() != "" { | ||
| source.TargetRevision = q.GetRevision() | ||
| } | ||
| sources = append(sources, source) |
There was a problem hiding this comment.
Since this part is common to both blocks, move it outside the if/else?
There was a problem hiding this comment.
Hii @crenshaw-dev, thanks for the review
made the changes. Also, about the build failure, it looks like a flaky test, could you confirm?
adityaraj178
force-pushed
the
diff-manifest-cmd-sourceHydrator
branch
from
eb9fcdb to
af0fd5f
Compare
Signed-off-by: Aditya Raj <[email protected]>
adityaraj178
force-pushed
the
diff-manifest-cmd-sourceHydrator
branch
from
af0fd5f to
f3a7839
Compare
| if a.Spec.SourceHydrator != nil { | ||
| source = a.Spec.SourceHydrator.GetDrySource() | ||
| } else { | ||
| source = a.Spec.GetSource() |
There was a problem hiding this comment.
| source = a.Spec.GetSource() | |
| source = a.Spec.GetSource() |
how will this work for a multi-source application?
There was a problem hiding this comment.
shouldn't there be changes to be made at line 514 to handle multi-source application logic?
There was a problem hiding this comment.
hii @nitishfy,
[HasMultipleSources()] returns false for SourceHydrator apps (since it checks spec.SourceHydrator == nil) and it handles multiple source case well
now single-source apps go to the else branch and here it got divided into sourceHydrator and non-sourceHydrator.
Multi-source apps will take the if branch(line 514) and never reach line 529.
Isn't sourceHydrator is mutually exclusive with multi-source, they can't be used together.
closes: #23817
Changes made:
Update the code so that when an Application has sourceHydrator configured the revision provided will be applied to the DrySource, not to the SyncSource.
Checklist: