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

Custom chain ID from env #970

Merged
merged 1 commit into from
Jul 19, 2022

Conversation

levackt
Copy link
Contributor

@levackt levackt commented Jul 19, 2022

CHAINID via env var doesn't work, it's always secretdev-1

@levackt
Copy link
Contributor Author

levackt commented Jul 19, 2022

The problem is the logic is inverted, so if the shorthand in the PR isn't preferred, we can fix it like so;

if [[ -z "${CHAINID}" ]]; then
    chain_id="secretdev-1"
else
    chain_id=$CHAINID
fi

@levackt
Copy link
Contributor Author

levackt commented Jul 19, 2022

Also present here, but probably only used by CI with defaults

@assafmo
Copy link
Member

assafmo commented Jul 19, 2022

Cool, didn't know of this syntax.

@levackt levackt force-pushed the fix/custom_local_chain_id branch from a0a446b to 9603773 Compare July 19, 2022 09:04
@Cashmaney
Copy link
Member

This breaks the secretjs tests for whatever reason - I assume it's because the dev image sets a chain id and now it gets improperly set (when the tests expect the default secretdev-1). @levackt mind taking a look? It should be in either the dev-image or release dockerfiles

@levackt
Copy link
Contributor Author

levackt commented Jul 19, 2022

Sure, let me check

@levackt levackt marked this pull request as draft July 19, 2022 13:32
@levackt levackt force-pushed the fix/custom_local_chain_id branch from c81182b to aa34613 Compare July 19, 2022 14:52
@levackt
Copy link
Contributor Author

levackt commented Jul 19, 2022

Tests look better, squashed the commits and undrafting, should build

@levackt levackt marked this pull request as ready for review July 19, 2022 14:55
@Cashmaney
Copy link
Member

LGTM! Thanks @levackt, merging this now

@Cashmaney Cashmaney merged commit b154384 into scrtlabs:master Jul 19, 2022
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.

3 participants