Add Mirroring and Credential UI #838

ikhoon · 2023-04-20T14:59:43Z

Motivation:

Creating and editing mirror settings require manual JSON editing which is time-consuming and error-prone. If a dedicated UI validates inputs and suggests default values such as mirrorable repositories and credentials, users will easily configure a mirror setting without making a mistake.

Related: #50

Modifications:

Server:
- Added dedicated REST APIs for mirror configurations and credential information. - Creating, listing, and updating operaitons are supported. - An array index is used as a key to update the existing configuration. This is an inevitable choice since the existing settings stored in an array and JSON patch can update an element based on index.
- Fixed AddOperation to create an JSON file if not exists.
- Added enabled property to MirrorCredential so that users can disable unused credentials.
- Added id property to Mirror so that users distinguish a mirror from others.
- Removed support for multiple mirror that is currenly not used. Should we deprecate MultipleMirrorConfig? #647
- Annotated @JsonProperties to the getters of MirrorCredential and its subclasses to serialize MirrorCredential to JSON as a REST API response.
Webapp:
- The following web pages were newly added for mirroring and credentials.
- Mirroring:
  - Create: /app/projects/{projectName}/mirrors/new
  - Update: /app/projects/{projectName}/mirrors/{index}/edit
  - List: /app/projects/{projectName}/mirrors
  - View: /app/projects/{projectName}/mirrors/{index}
- Credential:
  - Create: /app/projects/{projectName}/credentials/new
  - Update: /app/projects/{projectName}/credentials/{index}/edit
  - List: /app/projects/{projectName}/credentials
  - View: /app/projects/{projectName}/credentials/{index}
- Switched query-based tab URL for metadata to path-based to easily create breadcrumbs based on the path.
  - The old paths: /app/projects/metadata/{projectName}?tab=repositories|permissions}members|tokens|mirror
  - The new paths:
    - /app/projects/{projectName}/metadata
    - /app/projects/{projectName}/permissions - /app/projects/{projectName}/permissions/repos/{repoName} - /app/projects/{projectName}/members - /app/projects/{projectName}/tokens - /app/projects/{projectName}/mirrors - /app/projects/{projectName}/credentials
- Updated Breadcrumbs to replace a specific path segment name with another to display more meaningful information for breadcrumbs rather than showing an index number.

Result:

You can now use a web UI to set up a Git-to-CD mirror.
Closes UI for editing mirror settings #50

Motivation: Creating and editing mirror settings require manual JSON editing which is time-consuming and error-prone. If a dedicated UI validates inputs and suggests default values such as mirrorable repositories and credentials, users will easily configure a mirror setting without making a mistake. Related: line#50 Modifications: - Server: - Added dedicated REST APIs for mirror configurations and credential information. - Creating, listing, and updating operaitons are supported. - An array index is used as a key to update the existing configuration. This is an inevitable choice since the existing settings stored in an array and JSON patch can update an element based on index. - Fixed `AddOperation` to create an JSON file if not exists. - Added `enabled` property to `MirrorCredential` so that users can disable unused credentials. - Added `id` property to `Mirror` so that users distinguish a mirror from others. - Removed support for multiple mirror that is currenly not used. line#647 - Annotated `@JsonProperties` to the getters of `MirrorCredential` and its subclasses to serialize `MirrorCredential` to JSON as a REST API response. - Webapp: - The following web pages were newly added for mirroring and credential. - Mirroring: - Create: `/app/projects/{projectName}/mirrors/new` - Update: `/app/projects/{projectName}/mirrors/{index}/edit` - List: `/app/projects/{projectName}/mirrors` - View: `/app/projects/{projectName}/mirrors/{index}` - Credential: - Create: `/app/projects/{projectName}/credentials/new` - Update: `/app/projects/{projectName}/credentials/{index}/edit` - List: `/app/projects/{projectName}/credentials` - View: `/app/projects/{projectName}/credentials/{index}` - Switched query-based tab URL for metadata to path-based to easily create breadcrumbs based on the path. - The old paths: `/app/projects/metadata/{projectName}?tab=repositories|permissions}members|tokens|mirror` - The new paths: - `/app/projects/{projectName}/metadata` - `/app/projects/{projectName}/permissions` - `/app/projects/{projectName}/permissions/repos/{repoName}` - `/app/projects/{projectName}/members` - `/app/projects/{projectName}/tokens` - `/app/projects/{projectName}/mirrors` - `/app/projects/{projectName}/credentials` - Updated `Breadcrumbs` to replace a specific path segment name with another to display more meaning information to breadcrumbs rather than showing an index number. Result: - You can now use a web UI to set up a Git-to-CD mirror. - Closes line#50

ikhoon · 2023-04-20T16:28:15Z

Screenshots for the new UI

Credential form
Credential list
Credential view
Mirror form
Mirror list
Mirror view

ikhoon · 2023-04-20T16:35:09Z

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

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


MultipleMirrorConfig is removed and SingleMirrorConfig is migrated to MirrorConfig.java

I'd feel better if we migrate the internally used MultipleMirrorConfig before merging this PR (or at least before the next release so that the next release driver isn't suprised)

ikhoon · 2023-04-20T16:41:49Z

webapp/src/pages/_app.tsx

    // Next.js uses a path pattern to dynamically match the request path to a specific HTML file.
    // For example, `/app/projects/[projectNames]/index.html' is generated to render `/app/projects/myProj`
    // page. `FileService` serving the static resources for Central Dogma webapp does not understand the
    // path pattern syntax in the folder names. If `/app/projects/myProj` is requested, the `FileService` will
    // try to find '/app/projects/myProj/index.html' as is and fails, which in turn 'index.html' is returned as
    // a fallback. As a workaround, this triggers Next.js router to route to the desired page when a page is
    // loaded for the first time.
-    if (router.asPath !== '/') {
+    if (router.asPath !== '/' && router.isReady) {


If router.isReady is false, router.asPath could include a path pattern such as [projectName]. If router.isReady becomes true, the path pattern is replaced with the actual value like myProj.

jrhee17 · 2023-04-28T07:38:01Z

common/src/main/java/com/linecorp/centraldogma/internal/jsonpatch/AddOperation.java

        if (path.toString().isEmpty()) {
            return valueCopy();
        }
+        if (node.isNull() && Strings.isNullOrEmpty(path.head().getMatchingProperty()) &&


Question) Is there no need to add such logic to other operations such as MoveOperation, CopyOperation for the sake of consistency?

AddOperation could be performed on an empty node to add a new value.
However, MoveOperation and CopyOperation need an old value to perform. So I think it is abnormal behavior to run copy or move operation on an empty value.

I see, makes sense 👍

jrhee17 · 2023-04-28T07:53:03Z

server/src/main/java/com/linecorp/centraldogma/server/mirror/MirrorCredential.java

+     * Returns whether this {@link MirrorCredential} is enabled.
+     */
+    @JsonProperty("enabled")
+    boolean enabled();


Double checked that enabled is by default true if the value is null for both MirrorConfig, MirrorCredential

jrhee17 · 2023-04-28T08:00:24Z

...va/com/linecorp/centraldogma/server/internal/mirror/credential/AbstractMirrorCredential.java

+            // Web browsers may send an empty array `[]` as a value of hostnamePatterns
+            // which is converted into `[null]`.


Question) Is it possible for browsers to clean the input before sending such parameters? This feels unintuitive since the Web UI isn't the only code-path which uses this logic.

It is possible. I can inject a custom logic to covert an empty array to null for `hostnamePatterns.

jrhee17 · 2023-04-28T08:06:22Z

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

+                             remoteRepoUri.getScheme(),
+                             remoteRepoUri.getHost() + remoteRepoUri.getPath(),
+                             mirror.remotePath(),
+                             firstNonNull(mirror.remoteBranch(), "master"),


I'm not sure why we need to fallback to master. If I remember correctly, @trustin added a capability to detect the default branch by default if this value is null, and master is no longer the de-facto if due to main.

Is this necessary for the UI?

I guess we can leave it as null if a user specified null (= use default branch).

jrhee17 · 2023-04-28T08:08:48Z

server/src/main/java/com/linecorp/centraldogma/server/mirror/MirrorUtil.java

     * - remoteBranch:  master
     *
     * <p>e.g. dogma://foo.com/bar/qux.dogma is split into:
     * - remoteRepoUri: dogma://foo.com/bar/qux.dogma
     * - remotePath:    / (default)
     * - remoteBranch:  {@code defaultBranch}
     */
-    static String[] split(URI remoteUri, String suffix) {
+    public static String[] splitRemoteUri(URI remoteUri, String suffix, @Nullable String defaultBranch) {


Like mentioned in another comment, I'm unsure why this change is necessary 😅 Would you mind explaining/updating in the PR description?

Suggested change

public static String[] splitRemoteUri(URI remoteUri, String suffix, @Nullable String defaultBranch) {

public static String[] splitRemoteUri(URI remoteUri, String suffix) {

ikhoon · 2023-04-28T08:43:38Z

Please don't review the client part. The change is too huge that is hard to review.
It would be okay to q/a the features in a browser at the moment.

I will send a request to the main branch away. You can check the front-end code on the final PR if you want.

jrhee17 · 2023-04-28T08:27:04Z

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

+    // TODO(ikhoon): Automatically inject the non-optional query parameters to Route.matchesParam().
+    @MatchesParam("id")
+    @Get("/projects/{projectName}/credentials")
+    public CompletableFuture<MirrorCredential> getCredentialById(@Param String projectName, @Param String id) {


Question) Are credentialIds unique? I wasn't able to find any checks for duplicates, and I think users can still manually modify the credentials.json file anyways

I wasn't able to check the UI, but I'm wondering what happens if

a credential doesn't have an id

a credential has a duplicate id

jrhee17 · 2023-04-28T08:46:03Z

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

+            if (c == null) {
+                throw new RepositoryMetadataException(PATH_MIRRORS + " contains null.");
+            }
+            final Mirror mirror = c.toMirror(parent(), credentials, index++);


Question) What do you think of just persistently assigning an id? I understand that previously we didn't have mirrors, but I think it's possible to migrate to an object based schema instead of a mirror based schema and enforce ids and create an arbitrary id if missing.

With the current setup, I fear that users might make a mistake if multiple users modify mirror for the same project. i.e. Two users may try to modify a mirror configuration concurrently, and the index mapping might change.

jrhee17 · 2023-04-28T08:48:07Z

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

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


I'd feel better if we migrate the internally used MultipleMirrorConfig before merging this PR (or at least before the next release so that the next release driver isn't suprised)

ikhoon · 2023-04-28T08:56:29Z

Minwoo also worked for MultipleMirrorConfig removal at
#836 (comment)
#836 may be released before this. Otherwise, I will handle it separately.

jrhee17

Maybe I couldn't find it, but do we also provide a way to delete a mirror/credential? Or is enabled the equivalent of this?

jrhee17 · 2023-04-28T09:23:33Z

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

-        final Map<String, Entry<?>> entries =
-                find(new Revision(rev), PATH_CREDENTIALS_AND_MIRRORS, Collections.emptyMap()).join();
+    private CompletableFuture<Map.Entry<List<Mirror>, List<MirrorCredential>>> mirrorsAndCredentials(int rev) {
+        return find(new Revision(rev), PATH_CREDENTIALS_AND_MIRRORS, Collections.emptyMap()).thenApply(


Suggested change

return find(new Revision(rev), PATH_CREDENTIALS_AND_MIRRORS, Collections.emptyMap()).thenApply(

return find(new Revision(rev, PATH_CREDENTIALS_AND_MIRRORS).thenApply(

ikhoon · 2023-04-28T09:53:36Z

I don't think we can provide an API for removal because of the limitation of JSON patch that can update an array with its index. An index for an item should be consistent to provide a deep link for a mirror view and edit. Additionally, the size of Git mirrors and credentials isn't growing up to 100 or 1000. The enabled flag would be enough to manage those scales of data.

As a following work, we will hide mirrors.json and credentials.json from the file navigator of web UI and REST API retrieval so that users don't modify mirrors.json and credentials.json manually.

If removal for the mirror and credential information is really required, we can choose one of the following:

Migrate the array data format to object key-value format.
Add a custom JSON patch operation to update an array by a key field.

As you know, key / ID fields for mirrors and credentials is optional. A migration strategy is necessary to fill nullable IDs to implement the ideas.

trustin · 2023-04-28T11:42:57Z

As you know, key / ID fields for mirrors and credentials is optional. A migration strategy is necessary to fill nullable IDs to implement the ideas.

Agreed. Maybe we could auto-generate IDs and commit the changes automatically when it's read first time?

jrhee17

I see, I guess I didn't read the PR description carefully and didn't catch the intention 😅
I think there should be no problem with the current approach if the mirrors.json, credentials.json is not editable directly by end-users as a short-term solution 👍

I'm unsure how this will be rolled out with respect to the UI migration though (at the same time? vs. after UI migration is complete?)

jrhee17 · 2023-04-28T15:47:45Z

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

+                throw new EntryNotFoundException(
+                        "No such mirror at the index " + index + " in " + projectName);
+            }
+            return convertToMirrorDto(projectName, mirrors.get(index));


It seems like we're just using the index and not actually comparing mirrors.index with index from the parameters.
What do you think of just removing the index from the DTO altogether for simplicity if we know it's not going to change?

I didn't think there is any sorting from any point (since this is an array of json objects, there probably isn't a natural sorting order anyways)

trustin

What do you think about changing the directory layout of mirrors and credentials? e.g.

- mirrors
  - <mirror-id>.json
  - ...
- credentials
  - <credentials-id>.json
  - ...

At startup, we could:

perform automatic migration from mirrors.json and credentials.json to the new layout; and
rename mirrors.json and credentials.json to mirrors.json.bak and credentials.json.bak (so that the migration doesn't repeat.)

Then, all mirrors and credentials will have their unique IDs and we don't need to use integral index which is confusing.

I think this can be done as a follow-up task, though. (or we can do the migration first?) What do you think?

trustin · 2023-05-03T10:59:23Z

common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java

+        registerModules(new Jdk8Module(),
+                        new SimpleModule().addSerializer(Instant.class, InstantSerializer.INSTANCE)
                                          .addDeserializer(Instant.class, InstantDeserializer.INSTANT));


Not related to this PR, but should we do this in Armeria's DefaultJacksonObjectMapperProvider?

Armeria's default object mapper uses SPI to load all available modules. The new Java 8 API and time API are declared as dependencies.
https://github.com/line/armeria/blob/219ee3758b232ca9c47e03258a0a62f4cc4841c3/core/src/main/java/com/linecorp/armeria/internal/common/DefaultJacksonObjectMapperProvider.java#L42-L43
https://github.com/line/armeria/blob/70296b99b4277eef75da93ec85eeb26beec8d39e/core/build.gradle#L107-L108

trustin · 2023-05-03T11:01:20Z

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

+@ProducesJson
+@RequiresRole(roles = ProjectRole.OWNER)
+@ExceptionHandler(HttpApiExceptionHandler.class)
+public class CredentialServiceV1 extends AbstractService {


Suggested change

public class CredentialServiceV1 extends AbstractService {

public final class CredentialServiceV1 extends AbstractService {

trustin · 2023-05-03T11:08:23Z

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

+                             remoteRepoUri.getScheme(),
+                             remoteRepoUri.getHost() + remoteRepoUri.getPath(),
+                             mirror.remotePath(),
+                             firstNonNull(mirror.remoteBranch(), "master"),


I guess we can leave it as null if a user specified null (= use default branch).

trustin · 2023-05-03T11:13:32Z

server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java

+        return metadataRepo.push(projectName, Project.REPO_META, author, summary, change);
+    }
+
+    private static MirrorConfig converterToMirrorConfig(MirrorDto mirrorDto) {


Suggested change

private static MirrorConfig converterToMirrorConfig(MirrorDto mirrorDto) {

private static MirrorConfig convertToMirrorConfig(MirrorDto mirrorDto) {

trustin · 2023-05-03T11:14:09Z

server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java

+    private static MirrorConfig converterToMirrorConfig(MirrorDto mirrorDto) {
+        final String remoteUri =
+                mirrorDto.remoteScheme() + "://" + mirrorDto.remoteUrl() +
+                MirrorUtil.normalizePath(mirrorDto.remotePath()) + '#' + mirrorDto.remoteBranch();


#branch should be omitted if remoteBranch is null

ikhoon · 2023-05-03T12:10:57Z

What do you think about changing the directory layout of mirrors and credentials? e.g.

The design looks good. One thing to consider is mirror ID or credential ID are optional fields. To adopt this idea, some random IDs should be used to fill the null IDs

I will work on the migration first before merging this PR. The change of this is already huge. It would be easier to review if it is handled separately in advance.

ikhoon added the new feature label Apr 20, 2023

ikhoon added this to the revamp-webapp milestone Apr 20, 2023

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

Ignore unknown fields for mirror config

07e951a

ikhoon commented Apr 20, 2023

View reviewed changes

Clean up

fbf7cef

ikhoon commented Apr 20, 2023

View reviewed changes

jrhee17 reviewed Apr 28, 2023

View reviewed changes

trustin reviewed May 3, 2023

View reviewed changes

ikhoon mentioned this pull request Jul 26, 2023

Move mirror_state.json under meta repository #860

Open

ikhoon mentioned this pull request Sep 8, 2023

Introduce new mirroring and credential settings format and REST API #880

Draft

minwoox mentioned this pull request Mar 15, 2024

Update documentation as using Github Packages registry instead of Dockerhub #931

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Mirroring and Credential UI #838

Add Mirroring and Credential UI #838

ikhoon commented Apr 20, 2023 •

edited

ikhoon commented Apr 20, 2023

ikhoon Apr 20, 2023

jrhee17 Apr 28, 2023

ikhoon Apr 20, 2023

jrhee17 Apr 28, 2023

ikhoon Apr 28, 2023 •

edited

jrhee17 Apr 28, 2023

jrhee17 Apr 28, 2023

jrhee17 Apr 28, 2023

ikhoon Apr 28, 2023

jrhee17 Apr 28, 2023

trustin May 3, 2023

jrhee17 Apr 28, 2023

ikhoon commented Apr 28, 2023

jrhee17 Apr 28, 2023

jrhee17 Apr 28, 2023

jrhee17 Apr 28, 2023

ikhoon commented Apr 28, 2023

jrhee17 left a comment

jrhee17 Apr 28, 2023

ikhoon commented Apr 28, 2023 •

edited

trustin commented Apr 28, 2023

jrhee17 left a comment •

edited

jrhee17 Apr 28, 2023

trustin left a comment

trustin May 3, 2023

ikhoon May 4, 2023

trustin May 3, 2023

trustin May 3, 2023

trustin May 3, 2023

trustin May 3, 2023

ikhoon commented May 3, 2023 •

edited

		// Web browsers may send an empty array `[]` as a value of hostnamePatterns
		// which is converted into `[null]`.

	public static String[] splitRemoteUri(URI remoteUri, String suffix, @Nullable String defaultBranch) {
	public static String[] splitRemoteUri(URI remoteUri, String suffix) {

	return find(new Revision(rev), PATH_CREDENTIALS_AND_MIRRORS, Collections.emptyMap()).thenApply(
	return find(new Revision(rev, PATH_CREDENTIALS_AND_MIRRORS).thenApply(

	public class CredentialServiceV1 extends AbstractService {
	public final class CredentialServiceV1 extends AbstractService {

	private static MirrorConfig converterToMirrorConfig(MirrorDto mirrorDto) {
	private static MirrorConfig convertToMirrorConfig(MirrorDto mirrorDto) {

Add Mirroring and Credential UI #838

Are you sure you want to change the base?

Add Mirroring and Credential UI #838

Conversation

ikhoon commented Apr 20, 2023 • edited

ikhoon commented Apr 20, 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

ikhoon Apr 28, 2023 • edited

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 Apr 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon commented Apr 28, 2023

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon commented Apr 28, 2023 • edited

trustin commented Apr 28, 2023

jrhee17 left a comment • edited

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

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 May 3, 2023 • edited

ikhoon commented Apr 20, 2023 •

edited

ikhoon Apr 28, 2023 •

edited

ikhoon commented Apr 28, 2023 •

edited

jrhee17 left a comment •

edited

ikhoon commented May 3, 2023 •

edited