Run a mirror when its configuration is updated #1247

ikhoon · 2026-01-19T07:36:07Z

Motivation:

See #1183 for details.

Modifications:

Add remoteRevision, remotePath, localRevision and localPath fields to .mirror_state.json to capture the configuration used when a mirror is run.
Move RepositoryUri to public API and expose it in Mirror.java to allow access to the full remote URL in mirror tasks.
Set the depth of Git fetch to 2 to read parent commit.
- This is required to detect changes made on the remote in local-to-remote mode.
If mirrored data becomes dirty due to a manual push, it is overwritten in the next mirror job.

Result:

Git mirroring now runs correctly when the configuration changes or the mirrored content is manually overridden.
Closes Mirror is not updated when only the path has changed #1183

Motivation: See line#1183 for details. Modifications: - Add `remoteRevision`, `remotePath`, `localRevision` and `localPath` fields to `.mirror_state.json` to capture the configuration used when a mirror is run. - Move `RepositoryUri` to public API and expose it in `Mirror.java` to allow access to the full remote URL in mirror tasks. - Set the depth of Git fetch to 2 to read parent commit. - This is required to detect changes made on the remote in local-to-remote mode. Result: - Git mirroring now runs correctly when the configuration changes or the mirrored content is manually overridden. - Closes line#1183

coderabbitai · 2026-01-19T07:36:36Z

📝 Walkthrough

Walkthrough

Refactors mirror remote tracking from separate URI/path/branch fields into a unified RepositoryUri, expands MirrorState to include per-path remote/local mappings and configHash, and updates mirror logic (AbstractGitMirror) and tests to detect and act when only paths or mirror configs change.

Changes

Cohort / File(s)	Summary
Core mirror model & API `server/src/main/java/com/linecorp/centraldogma/server/internal/mirror/AbstractMirror.java`, `server/src/main/java/com/linecorp/centraldogma/server/mirror/RepositoryUri.java`, `server/src/main/java/com/linecorp/centraldogma/server/mirror/Mirror.java`	Replace three remote fields (URI, path, branch) with `RepositoryUri`; add `remoteUri()`; remove legacy getters; add cached hashString for object identity/toString.
Git mirror logic `server-mirror-git/src/main/java/.../AbstractGitMirror.java`, `server-mirror-git/src/main/java/.../DefaultGitMirror.java`, `server-mirror-git/src/main/java/.../SshGitMirror.java`, `server-mirror-git/src/test/java/.../MirrorSchedulingServiceTest.java`	Constructor parameterization to `RepositoryUri`; introduce MirrorState-driven decision flow (shouldRunRemoteToLocal / shouldRunLocalToRemote), increase fetch depth, compute previous commit, refine change detection and mirror_state.json handling.
Mirror state model `server/src/main/java/com/linecorp/centraldogma/server/internal/mirror/MirrorState.java`	Expand `MirrorState` to include remoteRevision, localRevision, direction, configHash; add accessors, equals/hashCode/toString, deprecate legacy sourceRevision accessor.
Providers & configuration `server-mirror-git/src/main/java/.../GitMirrorProvider.java`, `server/src/main/java/.../CentralDogmaMirror.java`, `server/src/main/java/.../CentralDogmaMirrorProvider.java`, `server/src/main/java/.../MirrorConfig.java`	Update provider/construction sites to pass `RepositoryUri` instead of separate uri/path/branch; adjust imports.
Integration & unit tests `it/mirror/src/test/java/.../git/GitMirrorIntegrationTest.java`, `it/mirror/src/test/java/.../git/LocalToRemoteGitMirrorTest.java`, `it/mirror-listener/src/test/java/.../CustomMirrorListenerTest.java`, `it/mirror/src/test/java/.../git/MirrorRunnerTest.java`, `server-mirror-git/src/test/java/.../MirrorSchedulingServiceTest.java`	Update tests to use `RepositoryUri`; add/adjust tests and helpers for per-path mirror mappings, per-path mirror_state.json expectations, and path-change-triggered scenarios; adjust expected messages.

Sequence Diagram(s)

sequenceDiagram
    participant Client as Mirror System
    participant Processor as AbstractGitMirror
    participant LocalRepo as Local Repository
    participant RemoteRepo as Remote Repository

    Client->>Processor: mirrorLocalToRemote()
    activate Processor
    Processor->>LocalRepo: read head commit & parent
    Processor->>RemoteRepo: fetch head commit (depth=2)
    Processor->>Processor: localCurrentMirrorState(), remoteCurrentMirrorState()
    Processor->>Processor: shouldRunLocalToRemote(oldState, localHead, remoteCommitId)
    alt Decision: RUN / COMPARE_AND_RUN
        Processor->>RemoteRepo: push changes for configured remotePath(s)
        Processor->>LocalRepo: write updated mirror_state.json (per-path, configHash)
        Processor-->>Client: mirror completed
    else Decision: SKIP
        Processor-->>Client: skip — up-to-date
    end
    deactivate Processor

    Client->>Processor: mirrorRemoteToLocal()
    activate Processor
    Processor->>RemoteRepo: fetch head commit (depth=2)
    Processor->>Processor: remoteCurrentMirrorState()
    Processor->>LocalRepo: read local mirror_state.json
    Processor->>Processor: shouldRunRemoteToLocal(oldState, localHead, remoteCommitId)
    alt Decision: RUN / COMPARE_AND_RUN
        Processor->>LocalRepo: apply remote changes to per-path localPath(s)
        Processor->>LocalRepo: write updated mirror_state.json (per-path, configHash)
        Processor-->>Client: mirror completed
    else Decision: SKIP
        Processor-->>Client: skip — up-to-date
    end
    deactivate Processor

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Provide a way to store variables via REST API #1243 — Modifies AbstractGitMirror and related mirror classes; touches similar areas of mirror decision logic and state handling.
Support for YAML #1229 — Updates usages and constructor signatures involving AbstractMirror/RepositoryUri; strongly related to API surface changes.

Suggested reviewers

trustin
minwoox
jrhee17

Poem

🐇 I hopped along the mirror trail,
Paths rearranged with every tail,
Now when a path slips out of view,
The mirror wakes and syncs anew,
Hooray — fresh reflections without fail!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 13.04% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main change: detecting and running mirrors when configuration is updated, which directly addresses the core issue.
Linked Issues check	✅ Passed	The PR fully implements the requirements from issue `#1183` by adding remoteRevision, remotePath, localRevision, and localPath fields to .mirror_state.json, promoting RepositoryUri to public API, and setting Git fetch depth to 2 to detect configuration and content changes.
Out of Scope Changes check	✅ Passed	All changes are scoped to implementing the mirror configuration update detection feature. Updates to AbstractMirror, MirrorState, RepositoryUri, and Mirror interface, along with test changes, are all directly related to detecting and running mirrors when configuration changes.
Description check	✅ Passed	The pull request description accurately describes the changes made: adding fields to mirror_state.json, promoting RepositoryUri to public API, setting Git fetch depth to 2, and enabling detection of configuration changes and manual overrides.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

jrhee17

👍 👍

jrhee17 · 2026-01-21T02:16:43Z

server/src/main/java/com/linecorp/centraldogma/server/internal/mirror/MirrorState.java

+     *
+     * @deprecated Use {@link #remoteRevision()} or {@link #localRevision()} instead.
+     */
+    @Deprecated


nit; the proposed logic always gets the previous revision of the target (local for remote2local, remote for local2remote).

It might be easier to understand if a separate previousTargetRevision() method is added.

jrhee17 · 2026-01-21T02:20:54Z

...or-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/AbstractGitMirror.java

        }
    }

+    boolean shouldRunMirror(@Nullable MirrorState oldMirrorState, Revision localHead,


Question) If there are multiple mirrors configured for a single repository, even if a file in directory A is changed will all mirrors be triggered due to also looking at the target revision?

I wonder if adding a hash of the mirror configuration for each mirror to MirrorState is a simpler/safer approach

This is an important point. I hadn't considered that multiple mirrors can be configured for a single repository.

if adding a hash of the mirror configuration for each mirror to MirrorState is a simpler/safer approach

A mirror configuration hash alone can't tell us whether we should run the mirror when there has been a manual push to the local repo. However, it would be worth adding the hash to detect any changes in mirror configuration.

Although, the logic is complex, the following approach seems to be the most appropriate under the Central Dogma revision model which does not provide the HEAD revision for a directory or a file.

Proposal: For remote-to-local mirroring, it should be safe to rely on the local HEAD when the local path is the root (/). If the local path is non-root (e.g, /subdir) and previousLocalRevision + 1 doesn't match the current HEAD, we could run repository.diff(previousLocalRevision, headRevision, "/subdir") to check whether there are actual changes happened before running a mirror task.

if adding a hash of the mirror configuration for each mirror to MirrorState is a simpler/safer approach

I interpreted the original issue as mirror doesn't run properly when the path (MirrorState) changes. I didn't think it was necessary that the revision of the target is considered for mirroring.

Whatever approach is taken, I'm fine as long as it's consistent for both directions.

I made new changes to properly overwrite a manual push in the next mirror job. PTAL.

jrhee17

👍 👍

jrhee17 · 2026-01-30T01:15:50Z

...or-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/AbstractGitMirror.java

+            return MirrorDecision.RUN;
+        }
+
+        if (!localHead.text().equals(previousLocalRevision)) {


Question) I may have not understood as the logic is complex, but is localHead ever equal to previousLocalRevision given that

localHead is the current HEAD revision

previousLocalRevision is from the MirrorState, which means it is the previous local revision?

jrhee17 · 2026-01-30T01:25:28Z

server/src/main/java/com/linecorp/centraldogma/server/internal/mirror/MirrorState.java

    @JsonCreator
-    MirrorState(@JsonProperty(value = "sourceRevision", required = true) String sourceRevision) {
+    public MirrorState(@JsonProperty(value = "sourceRevision", required = true) String sourceRevision,
+                       @JsonProperty("remoteRevision") @Nullable String remoteRevision,


Understood that:

For localToRemote

remoteRevision is the revision before the mirror push (so the previous version)

localRevision is the previous revision before mirror push

For remoteToLocal

remoteRevision is the current revision (since no push is done to remote)

localRevision is the previous revision before mirror push

It might be worth documenting this somewhere since I don't think this is well expressed in the currently proposed code.

jrhee17 · 2026-01-30T01:28:44Z

...or-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/AbstractGitMirror.java

+    }
+
+    private MirrorDecision shouldRunLocalToRemote(@Nullable MirrorState oldMirrorState, Revision localHead,
+                                                  @Nullable ObjectId remoteCommitId)


Optional) Naming if a commit id is previous or not may help understand the condition more clearly. Ditto for shouldRunRemoteToLocal

Suggested change

@Nullable ObjectId remoteCommitId)

@Nullable ObjectId prevRemoteCommitId)

where the previous prevRemoteCommitId could be mirror*

ikhoon added this to the 0.80.0 milestone Jan 19, 2026

ikhoon requested review from jrhee17, minwoox and trustin as code owners January 19, 2026 07:36

ikhoon added the defect label Jan 19, 2026

ikhoon marked this pull request as draft January 19, 2026 07:53

ikhoon added 2 commits January 19, 2026 18:18

fix tests

9301e69

lint

6a318f2

ikhoon marked this pull request as ready for review January 20, 2026 07:35

jrhee17 approved these changes Jan 21, 2026

View reviewed changes

ikhoon added 3 commits January 22, 2026 11:31

--wip-- [skip ci]

48be74e

Merge branch 'main' into fix-mirror-state

ce1b4b6

fix tests

5bcc64e

ikhoon requested a review from jrhee17 January 29, 2026 06:57

jrhee17 approved these changes Jan 30, 2026

View reviewed changes

	@Nullable ObjectId remoteCommitId)
	@Nullable ObjectId prevRemoteCommitId)

Run a mirror when its configuration is updated #1247

Are you sure you want to change the base?

Run a mirror when its configuration is updated #1247

Uh oh!

Conversation

ikhoon commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Uh oh!

jrhee17 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrhee17 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ikhoon commented Jan 19, 2026 •

edited

Loading

coderabbitai bot commented Jan 19, 2026 •

edited

Loading