centraldogma
centraldogma copied to clipboard
Introduce new mirroring and credential settings format and REST API
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.
- Mirror ID format:
- "id" is a required property in each configuration. Human-readable random words are used to create a unique ID.
- Change
Mirror
and related classes to haveid
,enabled
as required fields. - Add
CredentailServiceV1
andMirrorServiceV1
to serve REST API for CRU.- Create, read, and update operations are implemented in
DefaultMetaRepository
.
- Create, read, and update operations are implemented in
- 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, soMirror
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.
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.
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.
I found a bug in updating server's status during rolling restart. Let me send a separate PR to handle it first.
I will proceed with this PR after #921 is merged.
This PR is now ready for review again.
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.
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.
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.
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?
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 👍
@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?
Let me merge this to avoid conflicts with https://github.com/line/centraldogma/pull/965 Thanks a lot, @ikhoon 👍 👍 👍