-
Notifications
You must be signed in to change notification settings - Fork 46
ir: move config options out of top level #3619
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
base: master
Are you sure you want to change the base?
Conversation
da78f7f
to
aef9c30
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3619 +/- ##
==========================================
- Coverage 26.71% 26.69% -0.03%
==========================================
Files 653 653
Lines 50079 50111 +32
==========================================
- Hits 13380 13375 -5
- Misses 35656 35694 +38
+ Partials 1043 1042 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CHANGELOG.md
Outdated
`neofs-adm fschain estimations` was removed. | ||
|
||
Use new `fschain.disable_autodeploy` IR configuration option instead of deprecated `fschain_autodeploy`, | ||
that was removed in next release. |
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.
mention that default value for the old config has changed?
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.
but in fact, i think it is useless work, we will just drop it in the next release, nobody gonna change this exact value in their configs, they will just use the new one
pkg/innerring/innerring.go
Outdated
|
||
serveMetrics(server, cfg) | ||
|
||
// TODO: remove deprecated option in future releases |
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.
issue?
pkg/innerring/innerring.go
Outdated
serveMetrics(server, cfg) | ||
|
||
// TODO: remove deprecated option in future releases | ||
if cfg.IsSet("fschain_autodeploy") && cfg.IsSet("fschain.disable_autodeploy") && |
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 think any case when both values are set can be reported as an error, this is smth abnormal to me
aef9c30
to
1aff241
Compare
fschain_autodeploy
out of top levelNew `fschain.disable_autodeploy` IR configuration, which inverts the value of the `fschain_autodeploy` option. Make `fschain_autodeploy` default to `true`, support it for one more release. Closes #3361. Signed-off-by: Andrey Butusov <[email protected]>
Ref #3362. Signed-off-by: Andrey Butusov <[email protected]>
Ref #3362. Signed-off-by: Andrey Butusov <[email protected]>
Ref #3362. Signed-off-by: Andrey Butusov <[email protected]>
Closes #3362. Signed-off-by: Andrey Butusov <[email protected]>
1aff241
to
6f64aae
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.
seems good
if cfg.IsSet("fee.main_chain") { | ||
log.Warn("configuration option 'fee.main_chain' is deprecated, use 'mainnet.extra_fee' with the same value instead") | ||
// default value | ||
if cfg.Mainnet.ExtraFee == 50000000 { |
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.
default value may be specified in the config file explicitly. This will mutate it. We should be able to distinguish this case from Viper.SetDefault()
, and return an error
if cfg.IsSet("fschain_autodeploy") && cfg.IsSet("fschain.disable_autodeploy") { | ||
return nil, fmt.Errorf("'fschain_autodeploy' and 'fschain.disable_autodeploy' set simultaneously") | ||
} | ||
if cfg.IsSet("fschain_autodeploy") { |
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.
may be merged with the previous if
. Not a big deal taking into account further removal of both blocks
Closes #3361, #3362.