Skip to content

fix: network commands check for latest config version before build - #2922

Merged
aljo242 merged 10 commits into
developfrom
fix/networkchain-build-check-config-version
Oct 18, 2022
Merged

aljo242 merged 10 commits into
developfrom
fix/networkchain-build-check-config-version

Conversation

@jeronimoalbi

Copy link
Copy Markdown
Contributor

Fixes #2896

@lubtd lubtd left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor 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
Copy Markdown
Contributor 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.

@lubtd

lubtd commented Oct 14, 2022

Copy link
Copy Markdown
Contributor

Got it I haven't realize this

Thanks

@lubtd

lubtd commented Oct 14, 2022

Copy link
Copy Markdown
Contributor

Changes are good to me

Although Publish unit test seems broken

tbruyelle
tbruyelle previously approved these changes Oct 15, 2022
@jeronimoalbi

Copy link
Copy Markdown
Contributor 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
Copy Markdown
Contributor 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.

@lubtd

lubtd commented Oct 17, 2022

Copy link
Copy Markdown
Contributor

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

@lubtd lubtd left a comment

Copy link
Copy Markdown
Contributor

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
Comment thread changelog.md Outdated
Comment thread ignite/chainconfig/chainconfig.go Outdated
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