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: remove "kvStoreKeys" check cosmosanalysis.go #3871

Merged
merged 4 commits into from
Jan 14, 2024

Conversation

sashaduke
Copy link
Contributor

@sashaduke sashaduke commented Jan 4, 2024

Fixes #3850.

Since this function is only present in the app.go file of freshly scaffolded SDK v0.50+ blockchains, checking for this prevents backwards compatibility with blockchains scaffolded before SDK v0.50.
I have simply removed the line as it's not needed.

@github-actions github-actions bot added component:ci CI/CD workflow and automated jobs. component:configs component:packages labels Jan 4, 2024
@sashaduke sashaduke changed the title Remove "kvStoreKeys" check cosmosanalysis.go fix: remove "kvStoreKeys" check cosmosanalysis.go Jan 4, 2024
@julienrbrt
Copy link
Member

Hi, thanks for your PR, can you add e2e tests with a v0.47 app?
Additionally, can you add a changelog?

@sashaduke
Copy link
Contributor Author

Hi @julienrbrt, I'm not too familiar with your testing setup - are you referring to this? https://github.com/ignite/cli/tree/main/integration

Happy to add a changelog

@julienrbrt
Copy link
Member

julienrbrt commented Jan 8, 2024

Hi @julienrbrt, I'm not too familiar with your testing setup - are you referring to this? https://github.com/ignite/cli/tree/main/integration

Happy to add a changelog

Yes, can be here: https://github.com/ignite/cli/blob/main/integration/chain/cmd_serve_test.go.

Or maybe simpler and better, we can add a unit test for a v0.47 app.go here: https://github.com/ignite/cli/blob/v28.1.0/ignite/pkg/cosmosanalysis/cosmosanalysis_test.go / https://github.com/ignite/cli/blob/v28.1.0/ignite/pkg/cosmosanalysis/app/testdata

Please do add a changelog, thanks!

@sashaduke
Copy link
Contributor Author

@julienrbrt should be good to go now!

@julienrbrt
Copy link
Member

I guess this is fine to merge this and update the test in a follow-up.

@julienrbrt julienrbrt dismissed their stale review January 11, 2024 23:42

See above

@sashaduke
Copy link
Contributor Author

Sorry @julienrbrt, I've had lots on my plate this week I'm afraid. Happy to merge this now and come back to improve the tests when I have a bit more free time!

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

tACK.

@julienrbrt julienrbrt enabled auto-merge (squash) January 14, 2024 20:07
@julienrbrt julienrbrt merged commit 3565187 into ignite:main Jan 14, 2024
32 of 43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

app.go file cannot be found
2 participants