Skip to content

feat: implement history and rollback support for source hydrator (#27327)#27465

Open
liketosweep wants to merge 6 commits into
argoproj:masterfrom
liketosweep:clean-hydrator-feature
Open

feat: implement history and rollback support for source hydrator (#27327)#27465
liketosweep wants to merge 6 commits into
argoproj:masterfrom
liketosweep:clean-hydrator-feature

Conversation

@liketosweep
Copy link
Copy Markdown

What was the issue

The Rollback action in the History & Rollback UI had no effect on applications with Source Hydrator enabled. Triggering a rollback silently fell through the standard sync loop with no commit made to the syncBranch, forcing users to manually intervene via Git. This created significant friction during gitops-promoter + Source Hydrator onboarding workflows.

Fixes #27327


Why was it there

processRequestedAppOperation in the Application Controller had no interception logic for Source Hydrator rollback operations — rollback requests fell straight into the standard sync loop, which has no mechanism to fetch historical manifests and commit them to syncPath. Additionally, the hydrator.Dependencies interface had no RollbackApp method, meaning there was no defined contract for the hydrator to perform this operation at all.


My Approach to Solve It

  • Controller Interception: Added a guard block in controller/appcontroller.go inside processRequestedAppOperation that detects when a Source Hydrator app has a pending rollback operation. The block checks ctrl.hydrator != nil && app.Spec.SourceHydrator != nil && app.Operation != nil && app.Operation.Sync != nil, routes to RollbackApp, sets OperationState to Succeeded or Failed accordingly, and returns early — fully short-circuiting the standard sync loop.

  • RollbackApp Implementation: Implemented RollbackApp in controller/hydrator/hydrator.go. It resolves the target hydratedRevision from the operation, fetches the historical manifests from syncSource (not drySource), and commits them directly to syncBranch at syncPath via the commit server — replaying stored hydrated state rather than re-rendering from dry source, which would be unsafe if Application.spec had changed since the target revision.

  • Interface & Delegation: Added RollbackApp to the hydrator.Dependencies interface and added the corresponding delegation method on *ApplicationController in controller/appcontroller_hydrator.go to satisfy the interface contract.

  • Mock Update: Added RollbackApp to controller/hydrator/mocks/Dependencies.go to keep the testify mock in sync with the updated Dependencies interface, resolving the go vet failure in hydrator_test.go.

  • Scope Isolation: The rollback exclusively operates on the single application that triggered it. Sibling applications sharing the same syncBranch are unaffected — no cross-app side effects.


Result of the Fix

Users can now natively trigger rollbacks on Source Hydrator applications directly from the UI or CLI. The controller intercepts the operation, fetches the exact historical manifest state, and commits it to syncBranch — bypassing the standard hydration cycle entirely. The Application status reflects the outcome cleanly:

Phase:   Succeeded
Message: rolled back to <revision>

All go vet checks pass across controller/... and server/application/.... gofmt applied to all changed files.

Closes #27327

@liketosweep liketosweep requested a review from a team as a code owner April 20, 2026 20:48
@bunnyshell
Copy link
Copy Markdown

bunnyshell Bot commented Apr 20, 2026

❌ Preview Environment undeployed from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@liketosweep liketosweep marked this pull request as draft April 21, 2026 04:58
@liketosweep liketosweep marked this pull request as ready for review April 21, 2026 07:30
@liketosweep liketosweep force-pushed the clean-hydrator-feature branch from 667a2b3 to 91244d8 Compare April 21, 2026 07:30
@liketosweep
Copy link
Copy Markdown
Author

The only remaining failures are pre-existing E2E flakiness in test-hydrator-with-authenticated-repo-* identical timeout failures across all 4 versions (v1.32–v1.35), unrelated to this change.
@seankhliao would appreciate a review when you get a chance this implements the rollback support proposed in #27327

@liketosweep liketosweep force-pushed the clean-hydrator-feature branch from 91244d8 to 519d10b Compare April 22, 2026 21:43
@Qodo-Free-For-OSS
Copy link
Copy Markdown

Hi, processRequestedAppOperation treats any Sync operation on a SourceHydrator-enabled app as a rollback and returns early, preventing the normal sync loop from running. This breaks regular Sync requests (and retry flows that clear revision), and can cause unintended commits/incorrect success reporting.

Severity: action required | Category: correctness

How to fix: Gate rollback by explicit marker

Agent prompt to fix - you can give this to your LLM of choice:

Issue description

controller/ApplicationController.processRequestedAppOperation currently routes all Sync operations for SourceHydrator apps into hydrator.RollbackApp, preventing normal syncs and risking unintended rollback commits.

Issue Context

Normal syncs for single-source apps always populate Operation.Sync and typically set Operation.Sync.Source. Rollback needs an explicit discriminator.

Fix Focus Areas

  • Add an explicit rollback discriminator and gate interception on it (e.g., a dedicated Operation field, a SyncOption flag, or an Operation.Info marker set only by the Rollback API).

  • Ensure normal sync operations for SourceHydrator apps continue through the standard sync loop (SyncAppState).

  • Add a defensive check in the rollback path to reject empty revisions.

  • controller/appcontroller.go[1478-1494]

  • server/application/application.go[2263-2285]

  • server/application/application.go[2128-2143]

We noticed a couple of other issues in this PR as well - happy to share if helpful.


Found by Qodo code review

Copy link
Copy Markdown
Member

@choejwoo choejwoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested this locally?
The guard in processRequestedAppOperation has no discriminator between a rollback and a regular sync, so it would break normal syncs for any SourceHydrator-enabled app.
There are also a few more issues. The RollbackApp method added to the Dependencies interface (and its delegation in appcontroller_hydrator.go) is never actually called by the hydrator and looks like dead code, no guard against empty revision, and missing unit tests.
Please verify in your local env and attach some results.

@liketosweep liketosweep force-pushed the clean-hydrator-feature branch from 519d10b to 901264d Compare April 25, 2026 21:02
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 25, 2026

Codecov Report

❌ Patch coverage is 4.58716% with 104 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.47%. Comparing base (f48091a) to head (1910c4d).

Files with missing lines Patch % Lines
controller/hydrator/hydrator.go 5.08% 56 Missing ⚠️
controller/appcontroller.go 7.69% 22 Missing and 2 partials ⚠️
server/application/application.go 0.00% 23 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #27465      +/-   ##
==========================================
- Coverage   63.58%   63.47%   -0.11%     
==========================================
  Files         417      417              
  Lines       57062    57171     +109     
==========================================
+ Hits        36283    36291       +8     
- Misses      17386    17486     +100     
- Partials     3393     3394       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@liketosweep liketosweep marked this pull request as draft April 25, 2026 22:10
@liketosweep liketosweep force-pushed the clean-hydrator-feature branch 2 times, most recently from 4f6a097 to eb97cbd Compare April 26, 2026 08:26
@liketosweep liketosweep marked this pull request as ready for review April 26, 2026 11:27
@liketosweep
Copy link
Copy Markdown
Author

@choejwoo Thanks for the review. The implementation has been refactored to address the architectural and safety concerns:

Rollback Discriminator: Added an IsRollback marker to Operation.Info. processRequestedAppOperation now strictly gates rollback interception based on this flag, allowing standard SourceHydrator sync loops to proceed normally.

Interface Cleanup: Removed the dead RollbackApp method from the Dependencies interface and its delegation in appcontroller_hydrator.go. Regenerated the testing mocks via make codegen.

Defensive Guard & Coverage: Implemented a strict check against empty revisions in the hydrator path to prevent nil pointer panics (using errors.New for gofumpt/revive linter compliance). Added TestRollbackApp_EmptyRevision to guarantee coverage for this failure state.

image

Comment thread controller/hydrator/hydrator.go
@liketosweep liketosweep force-pushed the clean-hydrator-feature branch from eb97cbd to 9dcd99f Compare April 29, 2026 11:39
@liketosweep
Copy link
Copy Markdown
Author

@pbhatnagar-oss I have updated the implementation to address this.

The rollbackSource is now explicitly typed with Directory: &appv1.ApplicationSourceDirectory{} and pointed to the SyncSource path. This ensures that GetRepoObjs bypasses the rendering engine (Helm/Kustomize) and fetches the raw, previously hydrated manifests at the target revision, ensuring a deterministic rollback.

All CI checks, including unit and integration tests, are now passing. Ready for another review!

…ext)

Signed-off-by: liketosweep <liketosweep@gmail.com>
Signed-off-by: liketosweep <liketosweep@gmail.com>
Signed-off-by: liketosweep <liketosweep@gmail.com>
Signed-off-by: liketosweep <liketosweep@gmail.com>
Signed-off-by: liketosweep <liketosweep@gmail.com>
@liketosweep liketosweep force-pushed the clean-hydrator-feature branch from 9dcd99f to 1910c4d Compare April 29, 2026 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

History & Rollback support for Source Hydrator

4 participants