Provide a way to trigger Git mirroring manually #1041

ikhoon · 2024-09-30T15:08:20Z

Motivation:

There is no easy way to test Git mirroring configurations.
Some users may want to run Git mirroring manually instead of using the periodic scheduler.

Modifications:

Add POST /projects/{projectName}/mirrors/{mirrorId}/run REST API to MirroringServiceV1 so as to trigger a mirroring task.
Add verboseResponses to CentralDogmaConfig so as to contain full stack traces in error responses.
- The cause of mirroring failures will be displayed in the UI for administrators or verboseResponses is enabled.
Change AbstractMirror to return MirrorResult which is used to notify the result in the UI.
Make MirrorDto.schedule nullable.
- Exclude a mirror configuration from scheduling schedule it is null.
Move MirrorException to common and expose it in the error responses.
Add cronstrue dependency to package.json to display a human readable description on the mirror form UI.
Increase maxWidth of NotificationWrapper to render stack traces well.

Result:

You can now trigger a mirroring task in the mirror UI.
Fixes Provide a way to trigger Git mirroring immediately #700
Fixes Manual mirroring schedule #236

TBU

ikhoon · 2024-10-08T02:27:50Z

374385177-3dc05873-2e74-4bcf-b90d-d023acd196bc.mp4

jrhee17 · 2024-10-08T06:03:24Z

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

@@ -61,16 +62,18 @@ public abstract class AbstractMirror implements Mirror {
    private final String remoteBranch;
    @Nullable
    private final String gitignore;
+    @Nullable


Note: with this change, I feel like the enabled flag doesn't have much value.

e.g. If enabled = false but a manual trigger is done, will the mirror execute?

If enabled = false, the run button is disabled in the UI. However, I didn't reject the execution on the server side. Let me add the validation.

I feel like it still run even when enabled is false because the user might want to verify whether the mirror setting is correct.

Does that mean we check mirroring settings but do not push the mirrored data to the local repository when enabled = false?

I thought users could test a new mirror with enabled = true & scheduled = disabled settings.

jrhee17 · 2024-10-08T06:40:25Z

...ria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.java

@@ -133,6 +134,7 @@ public final class ArmeriaCentralDogma extends AbstractCentralDogma {
                        .put(RepositoryExistsException.class.getName(), RepositoryExistsException::new)
                        .put(InvalidPushException.class.getName(), InvalidPushException::new)
                        .put(ReadOnlyException.class.getName(), ReadOnlyException::new)
+                        .put(MirrorException.class.getName(), MirrorException::new)


Note: I understood this will actually never be called since CentralDogma client doesn't have any mirror related APIs

I added it mechanically. We may add mirror API in CentralDogma client later.

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

...main/java/com/linecorp/centraldogma/server/internal/api/auth/ApplicationTokenAuthorizer.java

jrhee17 · 2024-10-08T07:18:52Z

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

+                                                "mirrorApiWorker");
+    }
+
+    public CompletableFuture<MirrorResult> run(String projectName, String mirrorId) {


Optional) For better UX, if a mirrorRunId and the triggeredTime is returned I think it may be more clear to users whether the mirror result is from an in-flight mirror or a new attempt.

It is difficult to create a unique/incremental ID without an external system. However, triggeredTime can be easily added.

I was imagining a simple UUID inside MirrorKey, but I guess just returning triggeredTime is fine too

server/src/main/java/com/linecorp/centraldogma/server/internal/api/MirroringServiceV1.java

jrhee17 · 2024-10-08T07:28:11Z

site/src/sphinx/setup-configuration.rst

@@ -262,6 +263,11 @@ Core properties

    - the path of the management service. If not specified, the management service is mounted at ``/internal/management``.

+- ``verboseResponses`` (boolean)


Question) I'm not confident exceptions don't contain sensitive information (e.g. like credentials). If we do add this property, are you planning on enabling it in our in-house clusters by default?

Right. I intended to add this feature for use in our in-house systems.

The sensitive information of credentials is marked when it is serialized.

The permission manager will check if the caller is granted to access the information.

Even though sensitive information is exposed in error stack traces and delivered to the caller, I think, the caller will be a user who already has the appropriate permissions.

I see, if you are confident then would it make more sense to just enable by default instead of adding a flag?

I feel it's too verbose if we return the stack trace for all HTTP APIs.
What do you think about only returning the stack trace for specific APIs (e.g. mirroring) when accessed by non-admin users?

Later, the exception could be used to generate the client-side stack trace as we did in Armeria gRPC implementation.
https://github.com/line/armeria/blob/main/grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/AbstractServerCall.java#L571-L575

Fine-tuning is a good idea, but for now I think this option is enough for our use case.

minwoox

Looks great all in all. Left small suggestions. 😉

minwoox · 2024-10-14T10:35:12Z

package-lock.json

@@ -0,0 +1,21 @@
+{


Do these files have to be at the root-level?

minwoox · 2024-10-15T02:12:55Z

server/src/main/java/com/linecorp/centraldogma/server/internal/api/MirroringServiceV1.java

@@ -125,11 +133,27 @@ private CompletableFuture<PushResultDto> createOrUpdate(String projectName,
        });
    }

+    /**
+     * POST /projects/{projectName}/mirrors/{mirrorId}/run


I'm not sure if it works or not but how about using a custom verb?
POST /projects/{projectName}/mirrors/{mirrorId}:run
https://cloud.google.com/apis/design/custom_methods

Unfortunately, we can't use a path pattern for a custom verb but should use regular expressions. I didn't want to use regex for this sample path pattern.

Then, it's fine as is. I might give it a try to get it working one day.

minwoox · 2024-10-15T02:41:02Z

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

@@ -61,16 +62,18 @@ public abstract class AbstractMirror implements Mirror {
    private final String remoteBranch;
    @Nullable
    private final String gitignore;
+    @Nullable


I feel like it still run even when enabled is false because the user might want to verify whether the mirror setting is correct.

minwoox · 2024-10-15T02:56:53Z

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

+                                             mirrorConfig.maxNumBytesPerMirror());
+                    }, worker);
+            // Remove the inflight request when the mirror task is done.
+            future.handleAsync((unused0, unused1) -> inflightRequests.remove(mirrorKey));


nit: How about using handle or handleAsync with the worker instead of using common pool?

I didn't use handle intentionally because inflightRequests.remove could be called recursively inside inflightRequests.computeIfAbsent(). I wasn't confident that all supported JDK versions were safe for the operation.

Let me specify worker for the job.

minwoox · 2024-10-15T03:14:25Z

site/src/sphinx/setup-configuration.rst

@@ -262,6 +263,11 @@ Core properties

    - the path of the management service. If not specified, the management service is mounted at ``/internal/management``.

+- ``verboseResponses`` (boolean)


I feel it's too verbose if we return the stack trace for all HTTP APIs.
What do you think about only returning the stack trace for specific APIs (e.g. mirroring) when accessed by non-admin users?

…vide error reasons

ikhoon added 6 commits October 1, 2024 00:08

Provide a way to trigger Git mirroring manually

20690ea

TBU

Handle concurrent requests to prevent multiple mirroring executions

ee8ccc7

Improve error messages for easier debug

a0530ee

Add verboseResponses flag and write tests

19d095a

clean up

d34d63c

fix all broken tests

0c88798

ikhoon marked this pull request as ready for review October 7, 2024 11:03

ikhoon requested review from jrhee17, minwoox and trustin as code owners October 7, 2024 11:03

ikhoon added the new feature label Oct 7, 2024

ikhoon added this to the 0.71.0 milestone Oct 7, 2024

jrhee17 reviewed Oct 8, 2024

View reviewed changes

ikhoon added 2 commits October 8, 2024 17:28

Address comments by @jrhee17

48ce75c

Add triggeredTime

2c9cc86

jrhee17 approved these changes Oct 14, 2024

View reviewed changes

minwoox reviewed Oct 15, 2024

View reviewed changes

ikhoon added 4 commits October 15, 2024 16:48

remove package(-lock).json

c663284

specify worker

4eaeece

revert 'verboseResponses' option and add new mirror exceptions to pro…

8962e2c

…vide error reasons

move exceptions to public API

08722d2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a way to trigger Git mirroring manually #1041

Provide a way to trigger Git mirroring manually #1041

ikhoon commented Sep 30, 2024 •

edited

Loading

ikhoon commented Oct 8, 2024 •

edited by trustin

Loading

jrhee17 Oct 8, 2024

ikhoon Oct 8, 2024

minwoox Oct 15, 2024

ikhoon Oct 15, 2024

jrhee17 Oct 8, 2024

ikhoon Oct 8, 2024

jrhee17 Oct 8, 2024

ikhoon Oct 8, 2024 •

edited

Loading

jrhee17 Oct 14, 2024

jrhee17 Oct 8, 2024

ikhoon Oct 8, 2024 •

edited

Loading

jrhee17 Oct 14, 2024

minwoox Oct 15, 2024

ikhoon Oct 15, 2024

minwoox left a comment

minwoox Oct 14, 2024

minwoox Oct 15, 2024

ikhoon Oct 15, 2024

minwoox Oct 16, 2024

minwoox Oct 15, 2024

minwoox Oct 15, 2024

ikhoon Oct 18, 2024

minwoox Oct 15, 2024

		@@ -262,6 +263,11 @@ Core properties

		- the path of the management service. If not specified, the management service is mounted at ``/internal/management``.

		- ``verboseResponses`` (boolean)

Provide a way to trigger Git mirroring manually #1041

Are you sure you want to change the base?

Provide a way to trigger Git mirroring manually #1041

Conversation

ikhoon commented Sep 30, 2024 • edited Loading

ikhoon commented Oct 8, 2024 • edited by trustin Loading

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

ikhoon Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon Oct 8, 2024 • edited Loading

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 left a 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

ikhoon commented Sep 30, 2024 •

edited

Loading

ikhoon commented Oct 8, 2024 •

edited by trustin

Loading

ikhoon Oct 8, 2024 •

edited

Loading

ikhoon Oct 8, 2024 •

edited

Loading