-
Notifications
You must be signed in to change notification settings - Fork 117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update the default config as document #923
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please explain the rationale behind this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I didn't notice that. 😓 Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thanks a lot, @thachlp!
dist/src/conf/dogma.json
Outdated
"writeQuotaPerRepository": { | ||
"requestQuota": 5, | ||
"timeWindowSeconds": 1 | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
"writeQuotaPerRepository": { | |
"requestQuota": 5, | |
"timeWindowSeconds": 1 | |
}, |
; and set to null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ikhoon Any thought? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We haven't fully verified the feature yet. Let's keep it null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then, @thachlp Please set this to null as @ikhoon suggested. 🙇
centraldogma/site/src/sphinx/setup-configuration.rst
Lines 50 to 53 in a5eb412
"writeQuotaPerRepository": { | |
"requestQuota": 5, | |
"timeWindowSeconds": 1 | |
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks for the cleanup @thachlp 🙇 👍 🙇
@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.