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

Jsonnet config #52

Merged
merged 24 commits into from
Apr 29, 2022
Merged

Jsonnet config #52

merged 24 commits into from
Apr 29, 2022

Conversation

mmsqe
Copy link
Collaborator

@mmsqe mmsqe commented Apr 27, 2022

No description provided.

@mmsqe mmsqe marked this pull request as ready for review April 27, 2022 14:18
@tomtau tomtau requested a review from yihuang April 29, 2022 01:32
mmsqe added 3 commits April 29, 2022 09:49
* apply read symstem env with optional dotenv for .yaml
* export VALIDATOR1_MNEMONIC=$VALIDATOR1_MNEMONIC && VALIDATOR2_MNEMONIC=$VALIDATOR2_MNEMONIC COMMUNITY_MNEMONIC=$COMMUNITY_MNEMONIC SIGNER1_MNEMONIC=$SIGNER1_MNEMONIC SIGNER2_MNEMONIC=$SIGNER2_MNEMONIC CRONOS_ADMIN=$CRONOS_ADMIN IBC_CRO_DENOM=$IBC_CRO_DENOM && poetry run pystarport serve --data=./data --config=pystarport/tests/test_expansion/cronos_has_posix_no_dotenv(*.yaml, *.jsonnet)
pystarport/cluster.py Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
pystarport/expansion.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@yihuang yihuang left a comment

Choose a reason for hiding this comment

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

Looks good overall, just think we don't need to change the existing yaml test cases, just add new ones.
And could you add a changelog.

@mmsqe mmsqe requested a review from yihuang April 29, 2022 06:35
pystarport/cluster.py Outdated Show resolved Hide resolved
pystarport/cluster.py Outdated Show resolved Hide resolved
cronos_has_dotenv = parent / "cronos_has_dotenv.yaml"
cronos_no_dotenv = parent / "cronos_no_dotenv.yaml"
cronos_has_posix_no_dotenv = parent / "cronos_has_posix_no_dotenv.yaml"
for type in [".yaml", ".jsonnet"]:
Copy link
Collaborator

@yihuang yihuang Apr 29, 2022

Choose a reason for hiding this comment

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

Nit: looks like a good case to use https://docs.pytest.org/en/6.2.x/parametrize.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

need call function, cant use parameter ar

Copy link
Collaborator

Choose a reason for hiding this comment

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

it'll run your test case once for each parameter automatically.

Copy link
Collaborator Author

@mmsqe mmsqe Apr 29, 2022

Choose a reason for hiding this comment

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

seems can pass function as parameter

Copy link
Collaborator

@yihuang yihuang left a comment

Choose a reason for hiding this comment

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

LGTM.
Only one minor suggestion is use parametrized feature of pytest to structure the yaml/jsonnet test case.

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.

2 participants