-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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(runtime): remove appv1alpha1.Config
from runtime
#21042
Conversation
WalkthroughWalkthroughThe recent changes involve the removal of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant Module
User->>App: Initialize Application
App->>Module: Setup Application
Module-->>App: Configuration Loaded
App-->>User: Application Ready
sequenceDiagram
participant User
participant OldApp as "Old App"
participant OldModule as "Old Module"
participant NewApp as "New App"
participant NewModule as "New Module"
User->>OldApp: Initialize Old Application
OldApp->>OldModule: Setup Old Application
OldModule-->>OldApp: Old Configuration Loaded
OldApp-->>User: Old Application Ready
User->>NewApp: Initialize New Application
NewApp->>NewModule: Setup New Application
NewModule-->>NewApp: New Configuration Loaded
NewApp-->>User: New Application Ready
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
@julienrbrt your pull request is missing a changelog! |
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.
Do we still need this for anything? We could replace with the depinject gogo version
Not on main no, in v0.50 we had a query for the app config, but we killed it as it wasn't being used. |
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.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- runtime/app.go (2 hunks)
- runtime/module.go (3 hunks)
- runtime/v2/app.go (2 hunks)
- runtime/v2/module.go (2 hunks)
Files skipped from review due to trivial changes (1)
- runtime/v2/app.go
Additional context used
Path-based instructions (3)
runtime/v2/module.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/module.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/app.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (5)
runtime/v2/module.go (2)
Line range hint
51-60
:
Removal ofAppConfig
field approved.The removal of the
AppConfig
field from theAppInputs
struct is consistent with the PR objectives and helps streamline the configuration management.
Line range hint
62-73
:
Removal ofapp.appConfig
assignment approved.The removal of the
app.appConfig = inputs.AppConfig
assignment in theSetupAppBuilder
function is consistent with the removal of theAppConfig
field from theAppInputs
struct.runtime/module.go (2)
Line range hint
55-66
:
Removal ofAppConfig
field approved.The removal of the
AppConfig
field from theAppInputs
struct is consistent with the PR objectives and helps streamline the configuration management.
Line range hint
68-78
:
Removal ofapp.appConfig
assignment approved.The removal of the
app.appConfig = inputs.AppConfig
assignment in theSetupAppBuilder
function is consistent with the removal of theAppConfig
field from theAppInputs
struct.runtime/app.go (1)
Line range hint
18-35
:
Removal ofappConfig
field and import statement approved.The removal of the
appConfig
field from theApp
struct and its associated import statement is consistent with the PR objectives and helps streamline the configuration management.
…152) * build(deps): bump cosmossdk.io/depinject from 1.0.0-alpha.4 to 1.0.0 Bumps [cosmossdk.io/depinject](https://github.com/cosmos/cosmos-sdk) from 1.0.0-alpha.4 to 1.0.0. - [Release notes](https://github.com/cosmos/cosmos-sdk/releases) - [Changelog](https://github.com/cosmos/cosmos-sdk/blob/main/CHANGELOG.md) - [Commits](cosmos/cosmos-sdk@orm/v1.0.0-alpha.4...log/v1.0.0) --- updated-dependencies: - dependency-name: cosmossdk.io/depinject dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> * updates * hack until cosmos/cosmos-sdk#21042 --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr>
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 like a good evolution that config is not leaking into the app too
(cherry picked from commit 502450c)
(cherry picked from commit 502450c) # Conflicts: # runtime/module.go # runtime/v2/app.go # runtime/v2/module.go
…) (#21079) Co-authored-by: Julien Robert <julien@rbrt.fr>
…) (#21080) Co-authored-by: Julien Robert <julien@rbrt.fr> Co-authored-by: marbar3778 <marbar3778@yahoo.com>
* main: feat(log): remove core dependency and update core interface to be dependency free (#21045) chore: fix some comments (#21085) feat: simulate nested messages (#20291) chore(network): remove `DefaultConfigWithAppConfigWithQueryGasLimit` (#21055) fix(runtime): remove `appv1alpha1.Config` from runtime (#21042) feat(simapp/v2): Add store server to testnet init cmd (#21076) chore(indexer/postgres): update to changes on main (#21077) feat(schema/appdata): async listener mux'ing (#20879) ci: Use large box for 052 branch sims on CI (#21067) chore(all): replace all `fmt.Errorf` without paramters with `errors.New` (#21068)
…(#152) * build(deps): bump cosmossdk.io/depinject from 1.0.0-alpha.4 to 1.0.0 Bumps [cosmossdk.io/depinject](https://github.com/cosmos/cosmos-sdk) from 1.0.0-alpha.4 to 1.0.0. - [Release notes](https://github.com/cosmos/cosmos-sdk/releases) - [Changelog](https://github.com/cosmos/cosmos-sdk/blob/main/CHANGELOG.md) - [Commits](cosmos/cosmos-sdk@orm/v1.0.0-alpha.4...log/v1.0.0) --- updated-dependencies: - dependency-name: cosmossdk.io/depinject dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> * updates * hack until cosmos/cosmos-sdk#21042 --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr>
Description
Since depinject v1, we actually do not supply
appv1alpha.Config
when we useCompose
from a json: https://github.com/cosmos/cosmos-sdk/pull/20931/files#diff-27f72d7ebe452671125abb151c9e77c6b092536cfafcc734c4f10faade8d0d0dR23-R62.That poses an issue as runtime requires it: https://github.com/cosmosregistry/chain-minimal/actions/runs/10064766032/job/27822404477 / cosmosregistry/chain-minimal#152
AppConfig
actually does not need to be on runtime (as the app config service is gone), we should not require it to be available.This should be backported to v0.52 once #21041 is merged.
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...
!
in the type prefix if API or client breaking changeCHANGELOG.md
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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit