Clone once per repo for Argo CD app updates#2036
Conversation
ATGardner
force-pushed
the
noamgal/yos-335-argo-cd-steps-perform-a-full-clone-for-every-application
branch
from
2d7ed12 to
5dc9ab7
Compare
dmelnyk-octo
left a comment
dmelnyk-octo
left a comment
There was a problem hiding this comment.
The new logic looks good! I didn't noticed anything odd.
I left few minor comments mosty about the naming and posible redundancy of some fields in added PlannedApplication.
There was a problem hiding this comment.
It's a bit confusing to have CreateSourceUpdater public method on ApplicationSourceUpdater class. Maybe call the class ApplicationSourceUpdater factory?
|
|
||
| public ArgoCDApplicationDto Application { get; } | ||
| public ArgoCDGatewayDto Gateway { get; } | ||
| public Application ApplicationFromYaml { get; } |
There was a problem hiding this comment.
Double check if you need to bring the whole manifest type here. IDE tells me it's used just to get namespace via plan.ApplicationFromYaml.Metadata.Namespace that can also be take from Application DTO.
| public ArgoCDGatewayDto Gateway { get; } | ||
| public Application ApplicationFromYaml { get; } | ||
| public NamespacedApplicationName NamespacedName { get; } | ||
| public string ApplicationName { get; } |
There was a problem hiding this comment.
Also might be take from DTO or ApplicationFromYaml if keeping it here.
| public Application ApplicationFromYaml { get; } | ||
| public NamespacedApplicationName NamespacedName { get; } | ||
| public string ApplicationName { get; } | ||
| public IReadOnlyList<PlannedSource> Sources { get; } |
There was a problem hiding this comment.
MatchingSources might be a better name to eliminate any misinterpretation
ATGardner
force-pushed
the
noamgal/yos-335-argo-cd-steps-perform-a-full-clone-for-every-application
branch
from
5dc9ab7 to
ad197fb
Compare
Group sources across applications by repository and branch so each repo is cloned once and each branch checked out once. Direct pushes commit once per application and push once per branch group; pull request mode is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ATGardner
force-pushed
the
noamgal/yos-335-argo-cd-steps-perform-a-full-clone-for-every-application
branch
from
7f73315 to
016c28f
Compare
| { | ||
| // Captured before the push. Pushes are serial and the clone is fresh, so a rebase-on-push | ||
| // (which would rewrite this SHA) is not expected; see RepositoryWrapper.PushChanges. | ||
| applicationCommit = repository.GetHeadCommitResult(); |
There was a problem hiding this comment.
This captures the per-application PushResult before the grouped direct push has completed. That can report the wrong SHA if PushChanges hits the supported retry path: RepositoryWrapper.PushChanges fetches and rebases before retrying, which rewrites these local commits. The SourceUpdateResult/service messages would then point at pre-rebase SHAs that were never pushed, no?
3 participants
Group sources across applications by repository and branch so each repo is cloned once and each branch checked out once. Direct pushes commit once per application and push once per branch group; pull request mode is unchanged.
this does not require any server change