-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
Align NewConfig with AddFlags defaults #19254
base: main
Are you sure you want to change the base?
Conversation
server/embed/config.go
Outdated
AuthToken: DefaultAuthToken, | ||
BcryptCost: uint(bcrypt.DefaultCost), | ||
AuthTokenTTL: 300, | ||
SelfSignedCertValidity: 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.
We need to update the default value in AddFlags
: change the 1 with cfg.SelfSignedCertValidity
. This comment applies other fields as well.
Line 734 in c9045d6
fs.UintVar(&cfg.SelfSignedCertValidity, "self-signed-cert-validity", 1, "The validity period of the client and peer certificates, unit is year") |
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.
Done
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 was saying to change
fs.UintVar(&cfg.SelfSignedCertValidity, "self-signed-cert-validity", 1, "The validity period of the client and peer certificates, unit is year")
to
fs.UintVar(&cfg.SelfSignedCertValidity, "self-signed-cert-validity", cfg.SelfSignedCertValidity, "The validity period of the client and peer certificates, unit is year")
Also need to do similar changes for other fields you changed in this PR.
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.
Oh, sorry I misread your comment. I agree with the direction, however I worried about changing defaults by mistake that is easy to miss. So I proposed to make a minimal step that is easily verifiable by test.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
... and 23 files with indirect coverage changes @@ Coverage Diff @@
## main #19254 +/- ##
=======================================
Coverage 68.84% 68.85%
=======================================
Files 420 420
Lines 35693 35702 +9
=======================================
+ Hits 24573 24582 +9
+ Misses 9698 9692 -6
- Partials 1422 1428 +6 Continue to review full report in Codecov by Sentry.
|
f011488
to
5a556d8
Compare
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.
LGTM with a minor comment, thx
server/embed/config.go
Outdated
ExperimentalTxnModeWriteWithSharedBuffer: ExperimentalTxnModeWriteWithSharedBuffer, | ||
ExperimentalDistributedTracingAddress: ExperimentalDistributedTracingAddress, | ||
ExperimentalDistributedTracingServiceName: ExperimentalDistributedTracingServiceName, |
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.
ExperimentalTxnModeWriteWithSharedBuffer: ExperimentalTxnModeWriteWithSharedBuffer, | |
ExperimentalDistributedTracingAddress: ExperimentalDistributedTracingAddress, | |
ExperimentalDistributedTracingServiceName: ExperimentalDistributedTracingServiceName, | |
ExperimentalTxnModeWriteWithSharedBuffer: DefaultExperimentalTxnModeWriteWithSharedBuffer, | |
ExperimentalDistributedTracingAddress: DefaultExperimentalDistributedTracingAddress, | |
ExperimentalDistributedTracingServiceName: DefaultExperimentalDistributedTracingServiceName, |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahrtr, serathius The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
5a556d8
to
663dfc9
Compare
98079bd
to
7fbc830
Compare
Signed-off-by: Marek Siarkowicz <[email protected]>
7fbc830
to
93e9072
Compare
cc @ahrtr @fuweid @AwesomePatrol @siyuanfoundation