Extract mirroring implementation from the server module #836

minwoox · 2023-04-14T03:43:14Z

Motivation:
Previously, the server module was using jgit5 and jgit6 libraries together. It's because jgit6 requires JDK 11 as a minimum, so we needed jgit5 for JDK 8 client.
However, that might cause a dependency conflict problem such as #835. If we extract jgit6 specific API and implementations to a new module, we can avoid this problem.

Modifications:

Add MirrorProvider interface that creates a Mirror.
- MirrorProviders are discovered via SPI.
Created a new module called server-mirror-git and added GitMirrorProvider.
jgit6 is used if javaTestVersion is greater or equal to 11.
- jgit5 is sued if javaTestVersion is 8.
Clean up dependencies in build.gradle.
(Breaking)
- The type property, which was useless, in the mirroring configuration is now removed.
- Removed server-git6 module.

Result:

The server module is now correctly built and tested with the proper JGit libraries depending on the JDK test version.

codecov · 2023-04-14T03:54:01Z

Codecov Report

Patch coverage: 64.92% and project coverage change: +0.11% 🎉

Comparison is base (07a1fa6) 65.50% compared to head (0780b2d) 65.61%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #836      +/-   ##
============================================
+ Coverage     65.50%   65.61%   +0.11%     
- Complexity     3328     3350      +22     
============================================
  Files           356      358       +2     
  Lines         13962    13908      -54     
  Branches       1499     1494       -5     
============================================
- Hits           9146     9126      -20     
+ Misses         3964     3932      -32     
+ Partials        852      850       -2

Files Changed	Coverage Δ
...ntraldogma/server/command/AbstractPushCommand.java	`46.66% <ø> (ø)`
...inecorp/centraldogma/server/mirror/MirrorUtil.java	`75.00% <ø> (-0.76%)`	⬇️
...ntraldogma/server/internal/mirror/GitWithAuth.java	`58.59% <46.15%> (ø)`
...rver/internal/storage/repository/MirrorConfig.java	`57.14% <57.14%> (ø)`
...corp/centraldogma/server/mirror/MirrorContext.java	`64.00% <64.00%> (ø)`
...server/internal/storage/repository/MirrorUtil.java	`75.00% <75.00%> (ø)`
...ogma/server/internal/mirror/GitMirrorProvider.java	`84.61% <84.61%> (ø)`
...storage/repository/CentralDogmaMirrorProvider.java	`89.47% <89.47%> (ø)`
...centraldogma/server/internal/mirror/GitMirror.java	`81.58% <100.00%> (ø)`
...erver/internal/mirror/DefaultMirroringService.java	`69.56% <100.00%> (+2.17%)`	⬆️
... and 2 more

... and 8 files with indirect coverage changes

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

trustin · 2023-04-17T04:34:07Z

...or/src/main/java/com/linecorp/centraldogma/server/internal/mirror/DefaultMetaRepository.java

I'm not sure if it makes sense to move DefaultMetaRepository out of :server. How about introducing some SPI so :mirror can register itself as a handler for a certain set of URI schemes?

That's a good suggestion. Addressed.

settings.gradle

minwoox · 2023-04-17T09:39:34Z

...java/com/linecorp/centraldogma/server/internal/storage/repository/DefaultMetaRepository.java

-            @Type(value = SingleMirrorConfig.class, name = "single"),
-            @Type(value = MultipleMirrorConfig.class, name = "multiple")
-    })
+    @JsonSubTypes(@Type(value = SingleMirrorConfig.class, name = "single"))
    private abstract static class MirrorConfig {


Will combine MirrorConfig and SingleMirrorConfig together after @ikhoon's work for mirroring UI is done.

Combine them in this PR instead of waiting for @ikhoon's work. 😉

trustin · 2023-08-18T08:15:58Z

...it/src/main/java/com/linecorp/centraldogma/server/internal/mirror/DefaultMirrorProvider.java

+import com.linecorp.centraldogma.server.mirror.MirrorContext;
+import com.linecorp.centraldogma.server.mirror.MirrorProvider;
+
+public final class DefaultMirrorProvider implements MirrorProvider {


GitMirrorProvider?

trustin · 2023-08-18T08:16:56Z

...it/src/main/java/com/linecorp/centraldogma/server/internal/mirror/DefaultMirrorProvider.java

+
+        //noinspection EnhancedSwitchMigration
+        switch (scheme) {
+            case SCHEME_DOGMA: {


Could you file an issue about moving this back into the main server module and add a TODO comment that has the issue URL? This scheme doesn't belong to server-mirror-git.

Added CentralDogmaMirrorProvider and moved it back to the server module. 😅

trustin · 2023-08-18T11:15:32Z

...va/com/linecorp/centraldogma/server/internal/mirror/DefaultMetaRepositoryWithMirrorTest.java

        metaRepo.commit(Revision.HEAD, 0, Author.SYSTEM, "",
                        Change.ofJsonUpsert(
                                PATH_MIRRORS,
                                "[{" +
                                "  \"enabled\": true," +
-                                "  \"type\": \"single\"," +


Can we at least keep one entry with "type:": "single" to ensure the backward compatibility?

It's here. 😉 https://github.com/line/centraldogma/pull/836/files#diff-42445cf029e7cfb1d5a9f00593163dd61061d62631ab68e3f8200e56a626870dR233-R235

trustin · 2023-08-18T11:16:59Z

server/src/main/java/com/linecorp/centraldogma/server/command/AbstractPushCommand.java

@@ -81,7 +81,7 @@ public String detail() {
    }

    /**
-     * Returns the {@link Markup} of the {@link #detail()}.
+     * Returns the {@link Markup} of the {@code detail()}.


Maybe revert? This class still have #detail()

Revert and make AbstractPushCommand public so that Javadoc is correctly rendered.

trustin

👍

minwoox · 2023-08-23T02:33:21Z

Let me merge this and continue to work on #830. Please leave a comment here if there's something I need to fix. I will fix it in #830 together. 😉

ikhoon · 2023-08-23T10:22:24Z

server/build.gradle

-    implementation libs.jgit.ssh.jsch
+    // Hacky way to use different jGit versions depending on the test Java version.
+    // We will use jgit6 after we drop the support for Java 8 client.
+    if (project.ext.testJavaVersion >= 11) {


Sorry for the late review.
It is weird to change the target dependency which will be added to the POM file depending on testJavaVersion.
project.ext.targetJavaVersion should be a better choice.
If we want to test with jgit6, we can raise minimumJavaVersion to 11 or higher versions.
https://github.com/line/armeria/pull/4812/files#diff-79a2782078afe306b9bc1be1e6e5b3d1d94f9ef585741688c4d05bd4243569f5R4

If we use project.ext.targetJavaVersion, can we use jgit5 for testing the client module with testJavaVersion8?

Yes, it works.

# Use JGit 5 for testing. ~/src/centraldogma/client main* ⇣⇡ ❯ gw clean :client:java-armeria:test -PtestJavaVersion=11 ❯ ag "JGit version" java-armeria/build/test-results/test/TEST-com.linecorp.centraldogma.client.armeria.ArmeriaCentralDogmaTest.xml 57:2023-08-23 20:20:39.660 [INFO ](c.l.c.s.i.s.r.g.GitRepository) [repository-worker-6-1] JGit version: 5.13.0.202109080827-r java-armeria/build/reports/tests/test/classes/com.linecorp.centraldogma.client.armeria.ArmeriaCentralDogmaTest.html 146:2023-08-23 20:20:39.660 [INFO ](c.l.c.s.i.s.r.g.GitRepository) [repository-worker-6-1] JGit version: 5.13.0.202109080827-r # Use JGit 6 for testing ❯ gw clean :client:java-armeria:test -PtestJavaVersion=11 -PminimumJavaVersion=11 ❯ ag "JGit version" java-armeria/build/test-results/test/TEST-com.linecorp.centraldogma.client.armeria.ArmeriaCentralDogmaTest.xml 57:2023-08-23 20:20:08.281 [INFO ](c.l.c.s.i.s.r.g.GitRepository) [repository-worker-6-1] JGit version: 6.6.0.202305301015-r java-armeria/build/reports/tests/test/classes/com.linecorp.centraldogma.client.armeria.ArmeriaCentralDogmaTest.html 146:2023-08-23 20:20:08.281 [INFO ](c.l.c.s.i.s.r.g.GitRepository) [repository-worker-6-1] JGit version: 6.6.0.202305301015-r

By the way, I thought, an info message for the current JGit version would be useful.

static { InputStream is = null; try { is = SystemReader.class.getClassLoader().getResourceAsStream("META-INF/maven/org.eclipse.jgit/org.eclipse.jgit/pom.properties"); final Properties props = new Properties(); props.load(is); final Object jgitVersion = props.get("version"); if (jgitVersion != null) { logger.info("Using JGit: {}", jgitVersion); } } catch (IOException e) { logger.debug("Failed to read JGit version", e); } finally { if (is != null) { try { is.close(); } catch (IOException e) { // ignore } } } }

Let me send a PR for that.

Yes, it works.

Does it work when we specify -PtestJavaVersion=8? 🤔
Could you also share how did you implement the build.gradle?

Anyway, it's too late so let's keep discussing tomorrow. 😉

We don’t need to set an extra properties since we publish the server module with JGit 5.
JGit 6 will be chosen only mininumJavaVersion is higher than 11.

When packing a tarball, JGit 5 is excluded and JGit 6 is added instead.

Ah ha, now I understand. Well actually, publishing the server module with JGit 6 was my intention but I guess I was wrong because users would still need the server module for testing. Let me make a change, then. Thanks!

The change is pushed: 5118422
PTAL. 🙇

It looks 👍!

[WIP] Extract mirroring implementation from the server module

37e1a69

minwoox requested review from ikhoon, jrhee17 and trustin as code owners April 14, 2023 03:43

Fix grammar

d612fb7

minwoox added 2 commits April 14, 2023 13:16

Fix

ad7332d

Add documentation

f02c4ac

minwoox changed the title ~~[WIP] Extract mirroring implementation from the server module~~ Extract mirroring implementation from the server module Apr 17, 2023

misc

834f752

trustin reviewed Apr 17, 2023

View reviewed changes

minwoox added 2 commits April 17, 2023 17:01

Address the comment from @trustin

0f3c9e3

Reorder

9fbaed6

minwoox commented Apr 17, 2023

View reviewed changes

ikhoon mentioned this pull request Apr 28, 2023

Add Mirroring and Credential UI #838

Merged

minwoox added 2 commits August 18, 2023 15:39

Merge branch 'main' into extract_mirror

7d13815

Add missing snippet

c103d33

minwoox added this to the 0.61.6 milestone Aug 18, 2023

trustin reviewed Aug 18, 2023

View reviewed changes

minwoox added 3 commits August 18, 2023 18:03

Integrate MirrorConfig

342be32

Address comments from @trustin

c481f22

Add toString

3a6b347

minwoox added breaking change new feature labels Aug 18, 2023

trustin reviewed Aug 18, 2023

View reviewed changes

Add JGit version check

0780b2d

trustin approved these changes Aug 23, 2023

View reviewed changes

minwoox added improvement and removed new feature labels Aug 23, 2023

minwoox merged commit 0fd267d into line:main Aug 23, 2023
8 of 10 checks passed

minwoox deleted the extract_mirror branch August 23, 2023 02:33

ikhoon reviewed Aug 23, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract mirroring implementation from the server module #836

Extract mirroring implementation from the server module #836

minwoox commented Apr 14, 2023 •

edited

Loading

codecov bot commented Apr 14, 2023 •

edited

Loading

trustin Apr 17, 2023

minwoox Apr 17, 2023

minwoox Apr 17, 2023

minwoox Aug 18, 2023

trustin Aug 18, 2023

trustin Aug 18, 2023

minwoox Aug 18, 2023

trustin Aug 18, 2023

minwoox Aug 18, 2023

trustin Aug 18, 2023

minwoox Aug 18, 2023

trustin left a comment

minwoox commented Aug 23, 2023

ikhoon Aug 23, 2023

minwoox Aug 23, 2023

ikhoon Aug 23, 2023

ikhoon Aug 23, 2023

minwoox Aug 23, 2023 •

edited

Loading

minwoox Aug 23, 2023

ikhoon Aug 23, 2023 •

edited

Loading

minwoox Aug 23, 2023

minwoox Aug 23, 2023 •

edited

Loading

ikhoon Aug 23, 2023

Extract mirroring implementation from the server module #836

Extract mirroring implementation from the server module #836

Conversation

minwoox commented Apr 14, 2023 • edited Loading

codecov bot commented Apr 14, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

minwoox commented Aug 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox commented Apr 14, 2023 •

edited

Loading

codecov bot commented Apr 14, 2023 •

edited

Loading

minwoox Aug 23, 2023 •

edited

Loading

ikhoon Aug 23, 2023 •

edited

Loading

minwoox Aug 23, 2023 •

edited

Loading