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: network commands check for latest config version before build #2922

Merged
merged 10 commits into from
Oct 18, 2022

Conversation

jeronimoalbi
Copy link
Member

Fixes #2896

Copy link
Contributor

@lumtis lumtis left a comment

Choose a reason for hiding this comment

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

Does the PR should show an output like

config version v3 is required to run the command and the current version is v2

in case of config incompatibility?

If I publish a chain with an old config I get

ignite n chain publish https://github.com/ignite/example --hash 57e435b376d32c5dd35d325b4df2b1948e6cffae
Source code fetched
Blockchain set up
config is not valid: at least one validator is required

Same output as before

@jeronimoalbi
Copy link
Member Author

Highly likely that I missed a case that needs the validation. I got the right behaviour in my tests with a local SPN blockchain without using the --hash flag with:

ignite network chain publish --local github.com/ilgooz/mars

I will check it.

@jeronimoalbi
Copy link
Member Author

@lubtd this particular issue happens because the format of the config.yml in that changeset is invalid, it has version: 1 but the format is the one from v0. It should have a validators field to be v1 and still has a validator field from v0.

I will move the version check out from the chain build method though, because otherwise we could have cases that might scape the version check so I'm thinking that is better to check in each network command handler that uses the config.

@lumtis
Copy link
Contributor

lumtis commented Oct 14, 2022

Got it I haven't realize this

Thanks

@lumtis
Copy link
Contributor

lumtis commented Oct 14, 2022

Changes are good to me

Although Publish unit test seems broken

tbruyelle
tbruyelle previously approved these changes Oct 15, 2022
@jeronimoalbi
Copy link
Member Author

Changes are good to me

Although Publish unit test seems broken

@lubtd the test broke because is cloning the planet repo which has an outdated config. We could update the config in your planet repo but I see one "issue" which is that the repo would need to be updated each time the config version is upgraded in the CLI. Anyways it won't happen often I assume but it will become a chore :). Alternatively I can implement something so the config upgrade is done as part of the integration test. WDYT?

@jeronimoalbi
Copy link
Member Author

Regarding the last comment I am leaning towards updating the config of the planet repository each time the CLI config version changes but we need to use the --hash flag of the network chain publish command in the integration test to be sure that when eventually the config is updated in the CLI we don't break the integration tests for the other branches.
During a config version update in the CLI the TestNetworkPublish test will break making it "obvious" that the config must be updated in the planet repository.

Something worth mentioning for the current setup is that we would have to make a PR to the planet repo to change that config.

@lumtis
Copy link
Contributor

lumtis commented Oct 17, 2022

Mentioning the hash is a good idea

For https://github.com/lubtd/planet I think it was a temporary solution. Can we update the repo to https://github.com/ignite/example? It will make sense to keep the config of the repo up to date regularly as our typical example implementation of a chain with Ignite

Copy link
Contributor

@lumtis lumtis left a comment

Choose a reason for hiding this comment

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

As per my previous comment. Using https://github.com/ignite/example for the chain launch example for tests

tbruyelle
tbruyelle previously approved these changes Oct 18, 2022
Co-authored-by: Alex Johnson <alex@shmeeload.xyz>
@aljo242 aljo242 merged commit e2d49e4 into develop Oct 18, 2022
@aljo242 aljo242 deleted the fix/networkchain-build-check-config-version branch October 18, 2022 18:25
Jchicode pushed a commit to Jchicode/cli that referenced this pull request Aug 9, 2023
…gnite#2922)

* fix: network commands check for latest config version before build

* chore: change network publish integration test to use example repo

* Update changelog.md

Co-authored-by: Alex Johnson <alex@shmeeload.xyz>

* chore: improve config version check implementation

* test: add `chainconfig.CheckVersion` tests

Co-authored-by: Alex Johnson <alex@shmeeload.xyz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:network type:error Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't publish chain if config.yml is wrong for Ignite CLI
4 participants