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

fix!: change the instrumentation config to circumvent viper bug #1143

Merged
merged 7 commits into from
Nov 29, 2023

Conversation

evan-forbes
Copy link
Member

Description

There's an viper property or potential bug that stops us from overriding the default values for slices of strings. The "solution" atm is to use a string and parse it later like animals later. Unfortunately I think this is breaking since we're changing what a config is doing so tbh I'm not sure if we can ever include this in v1. If that's the case then we might want to consider not merging this to v0.34.x-celestia and only to main.

closes #1141

@evan-forbes evan-forbes self-assigned this Nov 28, 2023
@evan-forbes evan-forbes changed the title fix!: circumvent viper bug by using string instead of a slice of strings in config fix!: change the instrumentation config to circumvent viper bug Nov 28, 2023
rootulp
rootulp previously approved these changes Nov 28, 2023
Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

Unfortunately I think this is breaking since we're changing what a config is doing so tbh I'm not sure if we can ever include this in v1. If that's the case then we might want to consider not merging this to v0.34.x-celestia and only to main.

I'm in favor of merging this and cutting a new minor release (e.g. v1.30.0-tm-v0.34.29). I realize that technically breaks SemVer but I think the pro outweights the con.

Pro: feature parity between the branches we maintain
Con: breaking SemVer for releases on this repo

We expect the # of consumers of celestia-core to be few (ideally just celestia-app). Since we're responsible for bumping this dep in celestia-app, we also know that this minor release of celestia-core includes a breaking change.

config/toml.go Outdated Show resolved Hide resolved
config/toml.go Outdated Show resolved Hide resolved
@evan-forbes evan-forbes merged commit f1dc8f7 into v0.34.x-celestia Nov 29, 2023
17 checks passed
@evan-forbes evan-forbes deleted the evan/fix-viper-bug branch November 29, 2023 00:30
@evan-forbes
Copy link
Member Author

🤭 whoops thanks for the comment fix @rootulp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants