-
Notifications
You must be signed in to change notification settings - Fork 3.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
fix: Server config index-events incorrectly marshaled #10067
fix: Server config index-events incorrectly marshaled #10067
Conversation
Looks like a maintainer needs to give authorization in order for the CI workflows to run. https://github.com/cosmos/cosmos-sdk/actions/runs/1195217037
Once that's done, and they run, I'll check off the last |
Codecov Report
@@ Coverage Diff @@
## master #10067 +/- ##
==========================================
+ Coverage 63.56% 63.58% +0.02%
==========================================
Files 572 572
Lines 53584 53584
==========================================
+ Hits 34059 34074 +15
+ Misses 17582 17565 -17
- Partials 1943 1945 +2
|
server/config/toml.go
Outdated
tmpl := template.New("appConfigFileTemplate").Funcs(template.FuncMap{ | ||
"StringsJoin": strings.Join, | ||
}) |
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 reverted this line and the tests still passed, do we need this change? (im not to well versed in this side of the code so i could be very wrong)
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.
Good catch. I was copying stuff from the Tendermint config toml.go
and thought that was needed, but it's not. I've removed it and also added a unit test on the SetConfigTemplate
function to hopefully catch something being changed there and not init()
or vice versa.
For reference, that "StringsJoin"
is only used in the Tendermint config in one spot: rpc_servers = "{{ StringsJoin .StateSync.RPCServers "," }}"
. The StateSync.RPCServers
field is a []string
but looks like a single string in the config file. So that's not the correct thing to use for index-events
where we still want it to be a list of strings in the config file.
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.
ah i see! thank you for the explanation 😄
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.
tACK 👍🏻
Visit https://dashboard.github.orijtech.com?back=0&pr=10067&remote=false&repo=cosmos%2Fcosmos-sdk to see benchmark details. |
Thank you for the contribution @dwedul-figure! |
Description
Closes: #10016
Update the server config toml template to properly marshal the index-events string slice when not empty.
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...
added(not needed)!
to the type prefix if API or client breaking changefollowed the guidelines for building modules(not applicable)CHANGELOG.md
included comments for documenting Go code(not applicable)updated the relevant documentation or specification(not applicable)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...
!
in the type prefix if API or client breaking change