centraldogma icon indicating copy to clipboard operation
centraldogma copied to clipboard

Introduce new mirroring and credential settings format and REST API

Open ikhoon opened this issue 1 year ago • 3 comments

Motivation:

The ID for mirroring and credential configurations is optional, so I found it difficult to safely update a configuration with REST API. If a user updates a file manually on UI or commit API, the REST API may update a wrong configuration without a unique ID.

@trustin suggested changing the directory layout to store a configuration to a file with a unique ID. https://github.com/line/centraldogma/pull/838#pullrequestreview-1410657085

This PR has three major changes:

  • Migrate the old mirror and credential settings to the new layout.
    - mirrors
      - <mirror-id>.json
      - ...
    - credentials
      - <credentials-id>.json
      - ...
    
  • Add a migration job to automatically migrate the old settings to the new format when a server starts. The old files are renamed by adding .bak suffix. e,g. mirrors.json -> mirrors.json.bak, credentials.json -> credentials.json.bak.
  • Add REST API for mirroring and credential configurations. This is a necessary task to add mirror UI. #838

Modifications:

  • Add MirroringMigrationService that is executed when a server starts and scan all /mirrors.json and /credentials.json in the meta repo of projects and migrate them to the new format.
    • "id" is a required property in each configuration. Human-readable random words are used to create a unique ID.
      • Mirror ID format: mirror-<projectName>-<localRepo>-<shortWord>
      • Credential ID format: credential-<projectName>-<shortWord>
      • short_wordlist.txt is used as the word database.
  • Change Mirror and related classes to have id, enabled as required fields.
  • Add CredentailServiceV1 and MirrorServiceV1 to serve REST API for CRU.
    • Create, read, and update operations are implemented in DefaultMetaRepository.
  • Add RepositoryUri to represent a repository-specific URI such as a Git repository URL.
  • Add MirrorDto to serialize a mirroring configuration. Mirror represents a mirroring task, so Mirror is not suitable for serialization.
    • MirrorCredential is used as is instead of creating a new DTO.
  • Migrated all mirroring tests to use the new configuration format.
  • Updated site documentation with the new format.

Result:

  • Mirroring and credential settings have been updated to the new formats.
  • You can now access and modify mirroring and credential resources using the REST API.

ikhoon avatar Sep 06 '23 13:09 ikhoon

Codecov Report

Attention: Patch coverage is 71.81529% with 177 lines in your changes missing coverage. Please review.

Project coverage is 70.19%. Comparing base (aae194c) to head (28e1023). Report is 20 commits behind head on main.

:exclamation: Current head 28e1023 differs from pull request most recent head c128bf5

Please upload reports for the commit c128bf5 to get more accurate results.

Files Patch % Lines
...ver/internal/mirror/MirroringMigrationService.java 67.52% 54 Missing and 9 partials :warning:
...necorp/centraldogma/internal/api/v1/MirrorDto.java 46.77% 19 Missing and 14 partials :warning:
...rnal/storage/repository/DefaultMetaRepository.java 73.55% 20 Missing and 12 partials :warning:
...ver/internal/storage/repository/RepositoryUri.java 39.13% 14 Missing :warning:
...erver/internal/mirror/DefaultMirroringService.java 50.00% 10 Missing :warning:
...er/storage/project/InternalProjectInitializer.java 64.00% 5 Missing and 4 partials :warning:
...ldogma/server/internal/api/MirroringServiceV1.java 85.71% 4 Missing :warning:
...com/linecorp/centraldogma/server/CentralDogma.java 91.89% 1 Missing and 2 partials :warning:
...ecorp/centraldogma/server/CentralDogmaBuilder.java 0.00% 3 Missing :warning:
...corp/centraldogma/server/mirror/MirrorContext.java 71.42% 2 Missing :warning:
... and 4 more
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #880      +/-   ##
============================================
+ Coverage     66.86%   70.19%   +3.33%     
- Complexity     3529     3820     +291     
============================================
  Files           370      382      +12     
  Lines         14531    15357     +826     
  Branches       1563     1643      +80     
============================================
+ Hits           9716    10780    +1064     
+ Misses         3936     3617     -319     
- Partials        879      960      +81     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Sep 08 '23 07:09 codecov[bot]

I found a bug in updating server's status during rolling restart. Let me send a separate PR to handle it first.

ikhoon avatar Feb 14 '24 11:02 ikhoon

I will proceed with this PR after #921 is merged.

ikhoon avatar Feb 21 '24 13:02 ikhoon

This PR is now ready for review again.

ikhoon avatar Jun 12 '24 02:06 ikhoon

what do you think of allowing null ids when creating mirrors and credentials from a usability perspective.

ID is required to update the existing data. Previously, users updated the whole data such as mirrors.json or credentials.json at once. In the new UI, each configuration is updated separately, so we need ID to find the source file.

ikhoon avatar Jun 13 '24 10:06 ikhoon

CD server could generate random ids when no id is specified.

I missed this point. A mirror configuration can refer to a credential so credential's ID will be used as a foreign key. ID is also an identifier in the listing paging.

After I think about it in many ways, I still think users have to create and manage their own IDs.

ikhoon avatar Jun 13 '24 10:06 ikhoon

After I think about it in many ways, I still think users have to create and manage their own IDs.

Sure, I'm not sure what the UI will look like. If there is a good way for users to know in advance the used ids, I guess it will be fine.

jrhee17 avatar Jun 14 '24 00:06 jrhee17

I assume the changes will be released together with the new mirroring UI

This PR is going to be released before the mirroring UI. It may be difficult to detect bugs and handle them if this change was released along with the mirroring UI, so I wanted to release it separately.

Do you have any concerns about that?

ikhoon avatar Jun 14 '24 01:06 ikhoon

It may be difficult to detect bugs and handle them if this change was released along with the mirroring UI, so I wanted to release it separately.

Do you have any concerns about that?

I assume this comment is for the record since we already talked about this. I misread the code thinking reading/writing to the new mirrors/credentials files is not allowed anymore. Releasing separately sounds good 👍

jrhee17 avatar Jun 14 '24 02:06 jrhee17

@ikhoon Could you check the failure? https://github.com/line/centraldogma/actions/runs/9609269292/job/26503477206?pr=880#step:6:783 @trustin Could you take a look at this PR?

minwoox avatar Jun 24 '24 05:06 minwoox

Let me merge this to avoid conflicts with https://github.com/line/centraldogma/pull/965 Thanks a lot, @ikhoon 👍 👍 👍

minwoox avatar Jul 01 '24 05:07 minwoox