Skip to content
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

server: fix interceptConfigs #8641

Merged
merged 11 commits into from
Feb 22, 2021
Merged

server: fix interceptConfigs #8641

merged 11 commits into from
Feb 22, 2021

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Feb 19, 2021

Description

  1. cleanup error messages
  2. fix issue where app.toml was overriding config.toml

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

return nil, fmt.Errorf("failed to read in app.toml: %w", err)
rootViper.AddConfigPath(appCfgFilePath)

if err := rootViper.MergeInConfig(); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was causing any user value set in Tendermint's config to be wiped out. We need to merge configurations, not overwrite them.

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@alessio
Copy link
Contributor

alessio commented Feb 19, 2021

Argh - tests are failing :-/

@alexanderbez
Copy link
Contributor Author

Error: Canelled in prerun due to TestInterceptConfigsPreRunHandlerDoesNotMixConfigFiles failing?

@alexanderbez
Copy link
Contributor Author

So it seems TestInterceptConfigsPreRunHandlerDoesNotMixConfigFiles is actually wrong in the invariant its trying to test.

Recall, interceptConfigs reads configuration from config.toml (tendermint) and app.toml (application) and its intention is to merge them into a single viper instance.

So in TestInterceptConfigsPreRunHandlerDoesNotMixConfigFiles, where we check:

	if 0 != serverCtx.Viper.GetInt("halt-time") {
		t.Error("Halt time is not using default")
	}

But that is not the case.

/cc @hydrogen18 we should be using assertions via testfiy and no need for constants in these tests.

@hydrogen18
Copy link
Contributor

@alexanderbez is this PR related to an issue I opened? I don't really have any context here

@alexanderbez
Copy link
Contributor Author

alexanderbez commented Feb 19, 2021

@hydrogen18 its in relation to the TestInterceptConfigsPreRunHandlerDoesNotMixConfigFiles. We're trying to test and assert invariants that are not true. We should probably just remove this test all together

@hydrogen18
Copy link
Contributor

If what you're saying is correct, there's no difference between the two configuration files at all. You can specify any value in any file. Which leads me to the question of why have two files?

@alexanderbez
Copy link
Contributor Author

alexanderbez commented Feb 19, 2021

If what you're saying is correct, there's no difference between the two configuration files at all. You can specify any value in any file. Which leads me to the question of why have two files?

One is managed by Tendermint and the other by the application. What exists in the application is completely opaque to Tendermint and is completely customizable, so having one file would not make sense.

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm, utACK

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Feb 22, 2021
@codecov
Copy link

codecov bot commented Feb 22, 2021

Codecov Report

Merging #8641 (3e4f4c7) into master (7b52767) will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8641      +/-   ##
==========================================
- Coverage   61.47%   61.43%   -0.05%     
==========================================
  Files         659      658       -1     
  Lines       37876    37769     -107     
==========================================
- Hits        23283    23202      -81     
+ Misses      12159    12143      -16     
+ Partials     2434     2424      -10     
Impacted Files Coverage Δ
x/authz/genesis.go 66.66% <0.00%> (ø)
x/authz/types/msgs.go 53.84% <0.00%> (ø)
x/authz/types/codec.go 0.00% <0.00%> (ø)
x/authz/keeper/keeper.go 71.29% <0.00%> (ø)
x/authz/types/genesis.go 0.00% <0.00%> (ø)
x/authz/client/cli/query.go 75.90% <0.00%> (ø)
x/authz/types/generic_authorization.go 0.00% <0.00%> (ø)
x/authz/types/authorization_grant.go
x/staking/types/authz.go
x/bank/types/send_authorization.go
... and 3 more

@mergify mergify bot merged commit 2f069c9 into master Feb 22, 2021
@mergify mergify bot deleted the bez/fix-cfg-load branch February 22, 2021 11:51
mergify bot pushed a commit that referenced this pull request Feb 22, 2021
* server: fix interceptConfigs

* cl++

* revert call

* update error message

* revise TestInterceptConfigsPreRunHandlerDoesNotMixConfigFiles

* remove TestInterceptConfigsPreRunHandlerDoesNotMixConfigFiles

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 2f069c9)

# Conflicts:
#	CHANGELOG.md
#	server/util.go
alessio pushed a commit that referenced this pull request Feb 22, 2021
From: #8641

Co-authored-by: Aleksandr Bezobchuk <aleks.bezobchuk@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants