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

Add expansion feature #37

Merged
merged 10 commits into from
Dec 3, 2021
Merged

Conversation

damoncro
Copy link
Contributor

Expand .env if specified in yaml configuration

@damoncro
Copy link
Contributor Author

damoncro commented Nov 30, 2021

This PR would not affect the old yaml setting. It only reads the dotenv field (if exists), and expand the setting. System Env is also expanded.

Read dotenv file and expand based on env and system env
pystarport/cluster.py Outdated Show resolved Hide resolved
pystarport/expansion.py 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.

I recommend to allow user to override dotenv path in cli parameter and init_cluster parameter. it'll be more flexible for user to handle path issues.

@damoncro
Copy link
Contributor Author

The dotenv field will be poped out in the setting after the expansion to make minimal impact of the original design. And keep it very simple usage: just read the .env file and expand it.

@damoncro
Copy link
Contributor Author

I'll try override dotenv path in cli.

damoncro added a commit to damoncro/cronos that referenced this pull request Nov 30, 2021
After crypto-com/pystarport#37, would change
back to crypto-com/pystarport.
@yihuang
Copy link
Collaborator

yihuang commented Dec 1, 2021

The dotenv field will be poped out in the setting after the expansion to make minimal impact of the original design. And keep it very simple usage: just read the .env file and expand it.

I suggest don't expand at all when dotenv is not specified, to keep it backward compatible.

@damoncro
Copy link
Contributor Author

damoncro commented Dec 1, 2021

The dotenv field will be poped out in the setting after the expansion to make minimal impact of the original design. And keep it very simple usage: just read the .env file and expand it.

I suggest don't expand at all when dotenv is not specified, to keep it backward compatible.

Yes, it is what it can do now. It is very simple, just one if statement. https://github.com/crypto-com/pystarport/pull/37/files#diff-aa7a1723d585e834919ecbcc08a9aacad7402b6d0dc1a1f5b478ad17c44657b5R911. And I will add cli support as well.

1. `--dotenv`: an optional cli option for overriding the dotenv option in
yaml file
2. move the expansion codes to expansion.py and handle it in `expand_yaml`
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 1, 2021

This pull request introduces 2 alerts when merging f17ff16 into a127111 - view on LGTM.com

new alerts:

  • 1 for Redundant assignment
  • 1 for Wrong number of arguments in a call

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 1, 2021

This pull request introduces 2 alerts when merging 946d5e9 into a127111 - view on LGTM.com

new alerts:

  • 1 for Redundant assignment
  • 1 for Wrong number of arguments in a call

damoncro added a commit to damoncro/cronos that referenced this pull request Dec 2, 2021
After crypto-com/pystarport#37, would change
back to crypto-com/pystarport.
damoncro added a commit to damoncro/cronos that referenced this pull request Dec 2, 2021
After crypto-com/pystarport#37, would change
back to crypto-com/pystarport.
@damoncro
Copy link
Contributor Author

damoncro commented Dec 2, 2021

@damoncro
Copy link
Contributor Author

damoncro commented Dec 2, 2021

It is ready to merge. Please check the test result: https://github.com/crypto-com/pystarport/runs/4391781936?check_suite_focus=true

pyproject.toml Outdated
@@ -22,6 +22,9 @@ supervisor = "^4.2.1"
docker = "^4.3.1"
bech32 = "^1.1.0"
multitail2 = "^1.5.2"
python-dotenv = "^0.19.2"
deepdiff = "^5.6.0"
pytest = "^6.2.5"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be in the dev-dependencies?

@@ -902,9 +903,9 @@ def relayer_chain_config(data_dir, chain, relayer_chains_config):


def init_cluster(
data_dir, config_path, base_port, image=IMAGE, cmd=None, gen_compose_file=False
data_dir, config_path, base_port, dotenv, image=IMAGE, cmd=None, gen_compose_file=False
Copy link
Collaborator

@yihuang yihuang Dec 3, 2021

Choose a reason for hiding this comment

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

Suggested change
data_dir, config_path, base_port, dotenv, image=IMAGE, cmd=None, gen_compose_file=False
data_dir, config_path, base_port, dotenv=None, image=IMAGE, cmd=None, gen_compose_file=False

to keep it backward compatible.

@@ -1095,7 +1096,7 @@ def format_value(v, ctx):
if __name__ == "__main__":
interact("rm -r data; mkdir data", ignore_error=True)
data_dir = Path("data")
init_cluster(data_dir, "config.yaml", 26650)
init_cluster(data_dir, "config.yaml", 26650, None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
init_cluster(data_dir, "config.yaml", 26650, None)
init_cluster(data_dir, "config.yaml", 26650)

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 great, only minor suggestions, and could you add a changelog item.

@damoncro
Copy link
Contributor Author

damoncro commented Dec 3, 2021

Looks great, only minor suggestions, and could you add a changelog item.

Good

@yihuang yihuang self-requested a review December 3, 2021 04:27
@yihuang yihuang merged commit ed52d36 into crypto-com:main Dec 3, 2021
yihuang pushed a commit to crypto-org-chain/cronos that referenced this pull request Dec 6, 2021
…tests (#233)

* Add .env

* Load dotenv and expand it in all related files

1. Scritps
2. yaml
3. Python files

* Update to read full path of config files

* Test: Replace with test pystarport at first

After crypto-com/pystarport#37, would change
back to crypto-com/pystarport.

* Test expansion

Test the expansion feature

* Update the test pystarport

* Update test pystarport

* Update test_expansion

* Move the test_expansion to pystarport

* Back to crypto-com/pystarport@main

and remove deepdiff dependency.

* Use Path to get path

It is more general and work with full path as well

* Use full path of .env

* Use pystarport v0.2.3

* Override dotenv in start-cronos/start-chainmain

This makes these two script-bins defined in `scripts.nix` always use the
same yaml and dotenv file, for metting the "re-producibility"
requirement during one/same `nix-shell` environment.

These script-bins are wrappers of pystarport with fixed yaml and dotenv.

If you want to create the cronos/chainmain instance with different
configurations, you should avoid using the script-bins. Please run the
scripts directly, or use `pystarport` step by step.

Besides, if you change the yaml or dotenv, you need to exit and
re-enter the nix-shell. So that the modified yaml or dotenv are copied
to nix store, and their paths can be rewritten into these two
script-bins.
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.

None yet

2 participants