Support local-to-remote mirroring by minwoox · Pull Request #717 · line/centraldogma

minwoox · 2022-08-22T14:33:31Z

Motivation
We currently support only remote-to-local mirroring. We should support the opposite direction as well.

Modifications:

Implement GitMirror#mirrorToLocalRemote() which used to throw UnsupportedOperationException before.

Result:

Close CD-to-Git mirroring #53
You can now enable mirroring from Central Dogma to a remote Git server.

Motivation We currently support only remote-to-local mirroring. We should support the opposite direction as well. Modifications: - Implement `GitMirror#mirrorToLocalRemote()` which threw `UnsupportedOperationException` before. Result: - Close line#53 - You can now enable mirroring from Central Dogma to a remote Git server.

codecov · 2022-08-22T14:50:31Z

Codecov Report

Base: 64.95% // Head: 65.11% // Increases project coverage by +0.15% 🎉

Coverage data is based on head (52187d1) compared to base (0cf990c).
Patch coverage: 79.46% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #717      +/-   ##
============================================
+ Coverage     64.95%   65.11%   +0.15%     
- Complexity     3227     3259      +32     
============================================
  Files           349      349              
  Lines         13474    13670     +196     
  Branches       1454     1485      +31     
============================================
+ Hits           8752     8901     +149     
- Misses         3896     3928      +32     
- Partials        826      841      +15

Impacted Files	Coverage Δ
...erver/internal/mirror/DefaultMirroringService.java	`65.90% <0.00%> (ø)`
...ntraldogma/server/internal/mirror/MirrorState.java	`100.00% <ø> (ø)`
...inecorp/centraldogma/server/mirror/MirrorUtil.java	`76.47% <ø> (ø)`
...centraldogma/server/internal/mirror/GitMirror.java	`79.09% <80.00%> (-0.17%)`	⬇️
...necorp/centraldogma/testing/internal/TestUtil.java	`83.33% <100.00%> (+1.51%)`	⬆️
...aldogma/client/armeria/CompositeEndpointGroup.java	`75.00% <0.00%> (-18.75%)`	⬇️
...centraldogma/server/internal/api/WatchService.java	`76.27% <0.00%> (-3.39%)`	⬇️
.../linecorp/centraldogma/client/AbstractWatcher.java	`76.00% <0.00%> (-2.67%)`	⬇️
...ntraldogma/server/internal/mirror/GitWithAuth.java	`31.25% <0.00%> (+0.89%)`	⬆️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

ikhoon

Review in progress. Don't need to address the comments at the moment. I am going to review the remaining things tomorrow. Mostly looking good though. 😉

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

minwoox · 2022-09-26T14:50:01Z

@ikhoon Addressed all the comments. 😉

ikhoon · 2022-09-27T08:02:55Z

The master branch is merged into the working branch to run in Apple Silicon.

ikhoon

Left minor comments and questions.

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

ikhoon · 2022-09-27T12:44:44Z

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

If we eventually convert text into bytes, should we directly write JsonNode into bytes and use it as is? It would allocate fewer objects.

I thought we need to convert to String first because we need to use the pretty printer. If we use bytes, the mirror repository would see something like
{"a": {"foo":"bar"}} instead of

{ "a" : { "foo": "bar" } }

I see, I think this is reasonable:

For remote git repositories, we pretty print

For local storage, we save the compact json

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

minwoox

@ikhoon All addressed. PTAL. 😄

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

minwoox · 2022-10-05T06:17:01Z

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

I thought we need to convert to String first because we need to use the pretty printer. If we use bytes, the mirror repository would see something like
{"a": {"foo":"bar"}} instead of

{ "a" : { "foo": "bar" } }

jrhee17

Sorry about the late review 😅 I think this PR looks pretty much done 👍 Left some minor comments

jrhee17 · 2022-10-05T07:10:45Z

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

+
+                git.push()
+                   .setRefSpecs(new RefSpec(headBranchRefName))
+                   .setForce(true)


I think this can potentially overwrite commits. (e.g. multiple local repositories are pushed to a single remote repository, mis-operation can overwrite a repository, etc..)

If there is a conflict, what do you think of just failing the current iteration and allowing to update on the next iteration of mirror?

If there is a conflict, what do you think of just failing the current iteration and allowing to update on the next iteration of mirror?

That's a good suggestion. 👍

jrhee17 · 2022-10-18T06:35:30Z

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

minor nit; to avoid allocating an extra Stream object

Suggested change

final Stream<Map.Entry<String, Entry<?>>> entryStream =

localRawHeadEntries.entrySet()

.stream();

if (ignoreNode == null) {

// Use HashMap to manipulate it.

return entryStream.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));

}

if (ignoreNode == null) {

// Use HashMap to manipulate it.

return localRawHeadEntries;

}

final Stream<Map.Entry<String, Entry<?>>> entryStream = localRawHeadEntries.entrySet().stream();

We cannot just return localRawHeadEntries because it can be a singleton map that is not modifiable.
We do remove operation on the returned map.

jrhee17 · 2022-10-18T06:40:28Z

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

Question) Just curious, what happens if the branch doesn't exist in the remote repository? Will an exception be thrown? (as opposed to creating a branch if it doesn't exist)

I think it's fine to throw an exception, just want to make sure the behavior is known/defined.

I actually checked and verified an exception is thrown 👍

Thanks for checking it. 😉

jrhee17 · 2022-10-18T08:27:24Z

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

nit; technically String.length() isn't really the number of bytes (although I guess this is a more lenient upper-bound so this discrepancy isn't very critical)

Yeah, because it's not critical let's leave it as is.
Will get back and use
reader.getObjectSize(inserted, ObjectReader.OBJ_ANY) if it becomes a problem. 😉

jrhee17 · 2022-10-18T10:09:43Z

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

Q) Is it possible to use a common method for this logic? (with storage side)

It seems like this comment wasn't addressed. Is it possible to dedup this method?

centraldogma/server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java

Line 1166 in 0cf990c

private static String sanitizeText(String text) {

Ah forgot to leave a comment about this. 😄
I didn't intentionally dedup logic because of this PR #681
Let me cleanup the logic in the PR! 😉

jrhee17 · 2022-10-18T10:14:58Z

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

I see, I think this is reasonable:

For remote git repositories, we pretty print

For local storage, we save the compact json

jrhee17 · 2022-10-18T10:19:58Z

it/src/test/java/com/linecorp/centraldogma/it/mirror/git/LocalToRemoteGitMirrorTest.java

What do you think of also adding this to GitMirrorTest as well?

Suggested change

" \"remoteUri\": \"" + gitUri + firstNonNull(remotePath, "") + '"' +

" \"remoteUri\": \"" + gitUri + firstNonNull(remotePath, "") + '"' +

" \"schedule\": \"0 0 0 1 1 ? 2099\"," +

Thanks! Added. 😉

jrhee17

Left a minor comment but looks good to me 👍 Thanks @minwoox 👍 🙇 🚀 !

jrhee17 · 2022-10-20T07:29:30Z

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

jrhee17 · 2022-10-20T07:30:00Z

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

It seems like this comment wasn't addressed. Is it possible to dedup this method?

centraldogma/server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java

Line 1166 in 0cf990c

private static String sanitizeText(String text) {

ikhoon

Awesome!!! 🚀💯

it/src/test/java/com/linecorp/centraldogma/it/mirror/git/LocalToRemoteGitMirrorTest.java

minwoox · 2022-10-27T03:57:27Z

Thanks, @ikhoon and @jrhee17 for reviewing. 😉

minwoox added the new feature label Aug 22, 2022

minwoox requested review from ikhoon, jrhee17 and trustin as code owners August 22, 2022 14:33

minwoox added this to the 0.57.1 milestone Aug 22, 2022

Update document

665f77c

ikhoon reviewed Sep 1, 2022

View reviewed changes

minwoox added 6 commits September 26, 2022 16:36

Address comment by @ikhoon and add more test

7085f03

Add visible for testing annotation

6f9dfae

Close git

065b28e

Test deleing file

71ded38

Removing purging

fb6a18f

Revert TempDir

9e4d658

ikhoon reviewed Sep 27, 2022

View reviewed changes

minwoox added 2 commits October 5, 2022 14:49

Merge branch 'master' into cdToGit

5472052

Address comments by @ikhoon

e2c8db4

minwoox commented Oct 5, 2022

View reviewed changes

minwoox force-pushed the cdToGit branch from d3b8d90 to e2c8db4 Compare October 5, 2022 06:20

Merge branch 'master' into cdToGit

41a07b6

jrhee17 reviewed Oct 18, 2022

View reviewed changes

minwoox added 2 commits October 19, 2022 22:19

Address comments by @jrhee17

72a29e0

Merge branch 'master' into cdToGit

2bab7bf

minwoox force-pushed the cdToGit branch from eabe06a to 2bab7bf Compare October 19, 2022 13:22

jrhee17 approved these changes Oct 20, 2022

View reviewed changes

ikhoon approved these changes Oct 20, 2022

View reviewed changes

it/src/test/java/com/linecorp/centraldogma/it/mirror/git/LocalToRemoteGitMirrorTest.java Outdated Show resolved Hide resolved

Address the comment by @ikhoon

52187d1

minwoox merged commit 3baa748 into line:master Oct 27, 2022

minwoox deleted the cdToGit branch October 27, 2022 03:57

minwoox modified the milestones: 0.58.0, 0.57.4 Nov 8, 2022

-        final Stream<Map.Entry<String, Entry<?>>> entryStream =
-                localRawHeadEntries.entrySet()
-                                   .stream();
-        if (ignoreNode == null) {
-            // Use HashMap to manipulate it.
-            return entryStream.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
-        }
+        if (ignoreNode == null) {
+            // Use HashMap to manipulate it.
+            return localRawHeadEntries;
+        }
+        final Stream<Map.Entry<String, Entry<?>>> entryStream = localRawHeadEntries.entrySet().stream();

	" \"remoteUri\": \"" + gitUri + firstNonNull(remotePath, "") + '"' +
	" \"remoteUri\": \"" + gitUri + firstNonNull(remotePath, "") + '"' +
	" \"schedule\": \"0 0 0 1 1 ? 2099\"," +

Conversation

minwoox commented Aug 22, 2022

Uh oh!

codecov bot commented Aug 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ikhoon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

minwoox commented Sep 26, 2022

Uh oh!

ikhoon commented Sep 27, 2022

Uh oh!

ikhoon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

minwoox left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

jrhee17 left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 22, 2022 •

edited

Loading