Update the default config as document #923
Conversation
thachlp
requested review from
ikhoon,
jrhee17,
minwoox and
trustin
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #923 +/- ##
============================================
- Coverage 66.78% 66.77% -0.01%
+ Complexity 3513 3512 -1
============================================
Files 370 370
Lines 14476 14476
Branches 1553 1553
============================================
- Hits 9668 9667 -1
- Misses 3929 3930 +1
Partials 879 879 ☔ View full report in Codecov by Sentry. |
dist/src/conf/dogma.json
Outdated
| @@ -28,14 +28,18 @@ | |||
| }, | |||
| "webAppEnabled": true, | |||
| "webAppTitle": null, | |||
| "mirroringEnabled": null, | |||
| "mirroringEnabled": true, | |||
| "numMirroringThreads": null, | |||
| "maxNumFilesPerMirror": null, | |||
| "maxNumBytesPerMirror": null, | |||
| "replication": { | |||
| "method": "NONE" | |||
| }, | |||
| "csrfTokenRequiredForThrift": null, | |||
There was a problem hiding this comment.
I see this field csrfTokenRequiredForThrift in default config but not in documentation. Also the default configuration missing cors field
There was a problem hiding this comment.
Thanks. I added the cors field.
| @@ -28,14 +28,19 @@ | |||
| }, | |||
| "webAppEnabled": true, | |||
| "webAppTitle": null, | |||
| "mirroringEnabled": null, | |||
| "mirroringEnabled": true, | |||
There was a problem hiding this comment.
Could you please explain the rationale behind this change?
There was a problem hiding this comment.
I see in the documentation:
There was a problem hiding this comment.
Oops, I didn't notice that. 😓 Thanks!
dist/src/conf/dogma.json
Outdated
| "writeQuotaPerRepository": { | ||
| "requestQuota": 5, | ||
| "timeWindowSeconds": 1 | ||
| }, |
There was a problem hiding this comment.
I don't think this writeQuotaPerRepository is a sensible default.
Instead, what do you think of removing the settings in
centraldogma/site/src/sphinx/setup-configuration.rst
Lines 50 to 53 in a5eb412
; and set to null?
There was a problem hiding this comment.
Hi @ikhoon, from my view as a user, I think it is better to keep the documentation because it is easy to understand to set specific quotas. I should update this config to null.
Is it good?
There was a problem hiding this comment.
We haven't fully verified the feature yet. Let's keep it null.
There was a problem hiding this comment.
Then, @thachlp Please set this to null as @ikhoon suggested. 🙇
centraldogma/site/src/sphinx/setup-configuration.rst
Lines 50 to 53 in a5eb412
|
@thachlp 👍 👍 👍 |
Motivation
When I try to learn the centraldogma via document, I see that the default config of the source code is missing two properties from the document https://line.github.io/centraldogma/setup-configuration.html
So I create this pr.