feat: Allow app developers to override default appConfig template #9550

anilcse · 2021-06-21T08:04:00Z

Description

Closes: #5540

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

fedekunze · 2021-06-21T08:08:01Z

oh this is very handy! Thanks @anilcse! We had to fork the config and server in order to add support for the JSON-RPC. lmk if you need additional reviews. cc: @khoslaventures

codecov · 2021-06-21T08:13:45Z

Codecov Report

Merging #9550 (28a13df) into master (899b097) will increase coverage by 0.01%.
The diff coverage is 80.76%.

❗ Current head 28a13df differs from pull request most recent head 145b4be. Consider uploading reports for the commit 145b4be to get more accurate results

@@            Coverage Diff             @@
##           master    #9550      +/-   ##
==========================================
+ Coverage   60.60%   60.61%   +0.01%     
==========================================
  Files         589      589              
  Lines       37236    37256      +20     
==========================================
+ Hits        22567    22583      +16     
- Misses      12726    12730       +4     
  Partials     1943     1943

Impacted Files	Coverage Δ
server/config/toml.go	`13.33% <0.00%> (-4.85%)`	⬇️
simapp/simd/cmd/root.go	`66.89% <100.00%> (+5.17%)`	⬆️
x/authz/keeper/keeper.go	`66.93% <0.00%> (-1.04%)`	⬇️

fedekunze

Can you try running the server with the custom config? Usually, that's the part that involves the most changes when adding a custom config. See

https://github.com/tharsis/ethermint/blob/d8926636e7645e1a95f8bab409fd1118a58bfd1d/cmd/ethermintd/start.go#L241

simapp/simd/cmd/root.go

anilcse · 2021-06-21T09:19:09Z

Can you try running the server with the custom config? Usually, that's the part that involves the most changes when adding a custom config. See

https://github.com/tharsis/ethermint/blob/d8926636e7645e1a95f8bab409fd1118a58bfd1d/cmd/ethermintd/start.go#L241

You meant for adding a new server? This PR only deals with extra config settings (overriding app.toml). We would need to implement your modular server ADR for configuring new servers.

lgtm-com · 2021-06-21T09:27:05Z

This pull request introduces 1 alert when merging 17db7b4 into 6a5a2de - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

lgtm-com · 2021-06-21T10:20:15Z

This pull request introduces 1 alert when merging 9cd2021 into 6a5a2de - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

lgtm-com · 2021-06-21T11:41:45Z

This pull request introduces 1 alert when merging 7a5a3ad into 304d0ff - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

…s-sdk into anil/configurable_appconfig

alexanderbez

utACK. Nice work @anilcse 👍

server/config/toml.go

server/util.go

tac0turtle · 2021-06-21T13:57:08Z

simapp/simd/cmd/root.go

@@ -75,6 +79,43 @@ func NewRootCmd() (*cobra.Command, params.EncodingConfig) {
 	return rootCmd, encodingConfig
 }

+// initConfig helps to override default appConfig template and settings.
+// return; if no custom configuration is required for the application.
+func initConfig() (string, interface{}) {


could you add a few short sentences in the docs about this please.

oh sorry, meant the docs/ directory. I see you added in the doc.go.

I didn't find a relavant place to add it actually. There are no docs explaining server configs. May be I will add a new file or a new section under node docs.

updated rootCmd section docs

…s-sdk into anil/configurable_appconfig

tac0turtle

Amazing thank you

fedekunze

ACK. Would be great to add include this in v0.43

tac0turtle · 2021-08-12T10:00:53Z

simapp/simd/cmd/root.go

@@ -75,6 +78,45 @@ func NewRootCmd() (*cobra.Command, params.EncodingConfig) {
 	return rootCmd, encodingConfig
 }

+// initAppConfig helps to override default appConfig template and configs.
+// return "", nil if no custom configuration is required for the application.
+func initAppConfig() (string, interface{}) {


@anilcse do you think this could be reused to override tendermint config.toml?

I believe we can do that. I will look into it tomorrow and create a PR

…smos#9550)  ## Description Closes: cosmos#5540  --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [x] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [x] included comments for [documenting Go code](https://blog.golang.org/godoc) - [x] updated the relevant documentation or specification - [x] reviewed "Files changed" and left comments if necessary - [x] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)

anilcse added 3 commits June 16, 2021 14:14

init

221099d

Merge branch 'master' of github.com:cosmos/cosmos-sdk

56b3bfe

add hacky

08319d0

anilcse added 4 commits June 21, 2021 13:55

refactor

cf0daa1

Fix goimports

ef11f74

refactor

bb44743

add comment

3e5f8bb

anilcse marked this pull request as ready for review June 21, 2021 08:38

anilcse requested review from aaronc and alexanderbez as code owners June 21, 2021 08:38

Merge branch 'master' into anil/configurable_appconfig

9f83b5c

anilcse requested review from amaury1093, robert-zaremba, fedekunze, ethanfrey and tac0turtle June 21, 2021 08:38

anilcse changed the title ~~Allow app developers to override default appConfig template~~ feat: Allow app developers to override default appConfig template Jun 21, 2021

anilcse changed the title ~~feat: Allow app developers to override default appConfig template~~ !feat: Allow app developers to override default appConfig template Jun 21, 2021

anilcse changed the title ~~!feat: Allow app developers to override default appConfig template~~ feat: Allow app developers to override default appConfig template Jun 21, 2021

anilcse changed the title ~~feat: Allow app developers to override default appConfig template~~ !feat: Allow app developers to override default appConfig template Jun 21, 2021

anilcse added the T: Dev UX UX for SDK developers (i.e. how to call our code) label Jun 21, 2021

cleanup

17db7b4

fedekunze reviewed Jun 21, 2021

View reviewed changes

simapp/simd/cmd/root.go Outdated Show resolved Hide resolved

fix test

9cd2021

Merge branch 'master' into anil/configurable_appconfig

7a5a3ad

Fix golint

f80bca2

Merge branch 'anil/configurable_appconfig' of github.com:cosmos/cosmo…

4376792

…s-sdk into anil/configurable_appconfig

alexanderbez approved these changes Jun 21, 2021

View reviewed changes

server/config/toml.go Show resolved Hide resolved

server/util.go Outdated Show resolved Hide resolved

tac0turtle reviewed Jun 21, 2021

View reviewed changes

anilcse added 2 commits June 21, 2021 22:43

nits

ae8e5a9

update godoc

565c331

anilcse requested a review from tac0turtle June 21, 2021 17:28

anilcse added 5 commits June 21, 2021 22:58

Merge branch 'master' into anil/configurable_appconfig

118b215

Fix unmarshal

452bef6

Merge branch 'anil/configurable_appconfig' of github.com:cosmos/cosmo…

e73c64b

…s-sdk into anil/configurable_appconfig

add docs

4eea4ca

update commit hash

5474b17

anilcse changed the title ~~!feat: Allow app developers to override default appConfig template~~ feat: Allow app developers to override default appConfig template Jun 22, 2021

Merge branch 'master' into anil/configurable_appconfig

28a13df

tac0turtle approved these changes Jun 23, 2021

View reviewed changes

Merge branch 'master' into anil/configurable_appconfig

145b4be

fedekunze approved these changes Jun 23, 2021

View reviewed changes

fedekunze added the A:automerge Automatically merge PR once all prerequisites pass. label Jun 23, 2021

mergify bot merged commit 1c77766 into master Jun 23, 2021

mergify bot deleted the anil/configurable_appconfig branch June 23, 2021 08:42

This was referenced Jun 23, 2021

feat: Non-zero Default Fees #9371

Merged

feat: Non-zero Default Fees (backport #9371) #9582

Closed

yun-yeo mentioned this pull request Jun 29, 2021

[Bump] sdk version to v0.43.0-rc0 terra-money/classic-core#507

Merged

8 tasks

tac0turtle reviewed Aug 12, 2021

View reviewed changes

elise-ng mentioned this pull request Mar 14, 2022

Enable state sync by default in app.toml oursky/likecoin-chain#115

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow app developers to override default appConfig template #9550

feat: Allow app developers to override default appConfig template #9550

anilcse commented Jun 21, 2021 •

edited

Loading

fedekunze commented Jun 21, 2021

codecov bot commented Jun 21, 2021 •

edited

Loading

fedekunze left a comment

anilcse commented Jun 21, 2021 •

edited

Loading

lgtm-com bot commented Jun 21, 2021

lgtm-com bot commented Jun 21, 2021

lgtm-com bot commented Jun 21, 2021

alexanderbez left a comment

tac0turtle Jun 21, 2021

tac0turtle Jun 21, 2021

anilcse Jun 21, 2021

anilcse Jun 21, 2021

tac0turtle left a comment

fedekunze left a comment

tac0turtle Aug 12, 2021

anilcse Aug 12, 2021

tac0turtle Aug 12, 2021

feat: Allow app developers to override default appConfig template #9550

feat: Allow app developers to override default appConfig template #9550

Conversation

anilcse commented Jun 21, 2021 • edited Loading

Description

Author Checklist

Reviewers Checklist

fedekunze commented Jun 21, 2021

codecov bot commented Jun 21, 2021 • edited Loading

Codecov Report

fedekunze left a comment

Choose a reason for hiding this comment

anilcse commented Jun 21, 2021 • edited Loading

lgtm-com bot commented Jun 21, 2021

lgtm-com bot commented Jun 21, 2021

lgtm-com bot commented Jun 21, 2021

alexanderbez left a comment

Choose a reason for hiding this comment

tac0turtle Jun 21, 2021

Choose a reason for hiding this comment

tac0turtle Jun 21, 2021

Choose a reason for hiding this comment

anilcse Jun 21, 2021

Choose a reason for hiding this comment

anilcse Jun 21, 2021

Choose a reason for hiding this comment

tac0turtle left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

tac0turtle Aug 12, 2021

Choose a reason for hiding this comment

anilcse Aug 12, 2021

Choose a reason for hiding this comment

tac0turtle Aug 12, 2021

Choose a reason for hiding this comment

anilcse commented Jun 21, 2021 •

edited

Loading

codecov bot commented Jun 21, 2021 •

edited

Loading

anilcse commented Jun 21, 2021 •

edited

Loading