[fix] #1990: Enable peer startup via env vars in the absence of `config.json` #2188

ilchu · 2022-05-10T09:30:29Z

Signed-off-by: Ilia Churin ilia.ch@netogami.net

Description of the Change

In the case when the minimal cli client (iroha) was provided with no config.json either in the same directory or as an environment variable, do not stop the execution, but instead require that all the variables from the default json be provided as environment variables too, and load them onto default configuration.

Issue

Resolves #1990.

Benefits

Presumably easier and more flexible setup of peer through cli.

Possible Drawbacks

As the fix chooses to fall back on default impl for config, it has the same problems that were discussed in #2101 and #2118. The question is which variables should be allowed to be set via environment, and which subset of them should be required when using default, if we're going to stick with it at all. As of now, the default configuration is generated with Default and all the variables from the configs/peer/config.json are required to proceed if no file was found.

mversic · 2022-05-10T10:02:18Z

cli/src/lib.rs

@@ -143,7 +146,9 @@ where
        broker: Broker,
    ) -> Result<Self> {
        if !config.disable_panic_terminal_colors {
-            color_eyre::install()?;
+            if let Err(e) = color_eyre::install() {
+                iroha_logger::error!("Tried to install eyre_hook twice: {:?}", e);


why does this happen?

Was not able to trace the exact reason, but somehow with this workflow when no config was provided color_eyre kept crashing. AFAICT the only other spots where install() is used are kagami and logger, but they don't seem to be called at that stage. Will double check the error again, but this was a stumbling point for me.

I'm asking because I think this also happened to sdk devs when bootstraping iroha

Then at least it's not only me, but something in the code. I guess then as a workaround this will suffice for now, and I'll create a separate issue for the problem later today.

Kagami is a completely unrelated executable, so this is probably the only place this happens. Log and ignore for now, but we ned to figure out why this happens. IMO there's a tokio task that tries to do the same thing in a different thread and breaks the install process.

cli/src/main.rs

cli/README.md

cli/src/main.rs

cli/README.md

cli/src/main.rs

codecov · 2022-05-11T12:10:44Z

Codecov Report

Merging #2188 (e8bbee3) into iroha2-dev (89059b3) will decrease coverage by 76.65%.
The diff coverage is n/a.

❗ Current head e8bbee3 differs from pull request most recent head b3fe9f3. Consider uploading reports for the commit b3fe9f3 to get more accurate results

@@              Coverage Diff               @@
##           iroha2-dev   #2188       +/-   ##
==============================================
- Coverage       76.65%       0   -76.66%     
==============================================
  Files             185       0      -185     
  Lines           26672       0    -26672     
==============================================
- Hits            20445       0    -20445     
+ Misses           6227       0     -6227

Impacted Files	Coverage Δ
cli/src/lib.rs
cli/src/main.rs
core/src/smartcontracts/isi/query.rs
...ions_validators/src/public_blockchain/key_value.rs
telemetry/src/ws.rs
tools/kagami/src/main.rs
core/src/smartcontracts/mod.rs
client/examples/million_accounts_genesis.rs
actor/src/actor_id.rs
client/tests/integration/tx_rollback.rs
... and 175 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89059b3...b3fe9f3. Read the comment docs.

cli/src/main.rs

appetrosyan · 2022-05-11T12:24:41Z

cli/src/main.rs

+        "Additionally, in case of absence of both IROHA2_CONFIG_PATH and `config.json`
+in the current directory, all the variables from `config.json` should be set via the environment
+as follows:"
+    );


Explain that they need to passed as a JSON string. I'd add an example of that could be run from the command line in the comments/docs.

Added a paragraph in the amended commit describing that.

appetrosyan · 2022-05-11T13:27:35Z

cli/README.md

+The environment variables replacing `config.json` should be passed as JSON strings, meaning that any inner quotes should be properly escaped in the command line as shown in the rather unwieldy example below.
+
+``` bash
+IROHA_TORII="{\"P2P_ADDR\": \"127.0.0.1:1339\", \"API_URL\": \"127.0.0.1:8080\"}" IROHA_SUMERAGI="{\"TRUSTED_PEERS\": [{\"address\": \"127.0.0.1:1337\",\"public_key\": \"ed01201c61faf8fe94e253b93114240394f79a607b7fa55f9e5a41ebec74b88055768b\"},{\"address\": \"127.0.0.1:1338\",\"public_key\": \"ed0120cc25624d62896d3a0bfd8940f928dc2abf27cc57cefeb442aa96d9081aae58a1\"},{\"address\": \"127.0.0.1:1339\",\"public_key\": \"ed0120faca9e8aa83225cb4d16d67f27dd4f93fc30ffa11adc1f5c88fd5495ecc91020\"},{\"address\": \"127.0.0.1:1340\",\"public_key\": \"ed01208e351a70b6a603ed285d666b8d689b680865913ba03ce29fb7d13a166c4e7f1f\"}]}" IROHA_KURA="{\"INIT_MODE\": \"strict\",\"BLOCK_STORE_PATH\": \"./blocks\"}" IROHA_BLOCK_SYNC="{\"GOSSIP_PERIOD_MS\": 10000,\"BATCH_SIZE\": 2}" IROHA_PUBLIC_KEY="ed01201c61faf8fe94e253b93114240394f79a607b7fa55f9e5a41ebec74b88055768b" IROHA_PRIVATE_KEY="{\"digest_function\": \"ed25519\",\"payload\": \"282ed9f3cf92811c3818dbc4ae594ed59dc1a2f78e4241e31924e101d6b1fb831c61faf8fe94e253b93114240394f79a607b7fa55f9e5a41ebec74b88055768b\"}" IROHA_GENESIS="{\"ACCOUNT_PUBLIC_KEY\": \"ed01204cffd0ee429b1bdd36b3910ec570852b8bb63f18750341772fb46bc856c5caaf\",\"ACCOUNT_PRIVATE_KEY\": {\"digest_function\": \"ed25519\",\"payload\": \"d748e18ce60cb30dea3e73c9019b7af45a8d465e3d71bcc9a5ef99a008205e534cffd0ee429b1bdd36b3910ec570852b8bb63f18750341772fb46bc856c5caaf\"}}" ./iroha 


This is a little ugly, but this is meant to be technical documentation. Add some <details> tags around it and we can merge.

Signed-off-by: Ilia Churin <ilia.ch@netogami.net>

…ce of config.json (hyperledger#2188) Signed-off-by: Ilia Churin <ilia.ch@netogami.net>

…ce of config.json (hyperledger#2188) Signed-off-by: Ilia Churin <churin.ilya@gmail.com>

Signed-off-by: Ilia Churin <churin.ilya@gmail.com>

ilchu requested review from s8sato, appetrosyan, mversic, Arjentix, SamHSmith, QuentinI and outoftardis as code owners May 10, 2022 09:30

github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label May 10, 2022

mversic requested changes May 10, 2022

View reviewed changes

ilchu force-pushed the cli-env-vars branch 3 times, most recently from c493f24 to 166c5bf Compare May 10, 2022 11:43

Arjentix reviewed May 10, 2022

View reviewed changes

cli/src/main.rs Outdated Show resolved Hide resolved

cli/README.md Outdated Show resolved Hide resolved

Arjentix self-assigned this May 10, 2022

appetrosyan assigned ilchu May 11, 2022

mversic previously approved these changes May 11, 2022

View reviewed changes

ilchu dismissed mversic’s stale review via b9911bb May 11, 2022 09:10

ilchu force-pushed the cli-env-vars branch 4 times, most recently from 53199e0 to a7260ed Compare May 11, 2022 10:20

ilchu requested review from mversic and Arjentix May 11, 2022 10:46

outoftardis reviewed May 11, 2022

View reviewed changes

cli/README.md Outdated Show resolved Hide resolved

cli/src/main.rs Outdated Show resolved Hide resolved

cli/src/main.rs Outdated Show resolved Hide resolved

ilchu force-pushed the cli-env-vars branch 2 times, most recently from 5731324 to 6b34b1c Compare May 11, 2022 11:52

appetrosyan reviewed May 11, 2022

View reviewed changes

ilchu force-pushed the cli-env-vars branch 2 times, most recently from e8bbee3 to 7da5290 Compare May 11, 2022 13:18

appetrosyan requested review from appetrosyan and outoftardis May 11, 2022 13:26

appetrosyan reviewed May 11, 2022

View reviewed changes

ilchu force-pushed the cli-env-vars branch from 7da5290 to fe57d6e Compare May 11, 2022 13:35

appetrosyan approved these changes May 11, 2022

View reviewed changes

Enable peer startup in the absence of config

b3fe9f3

Signed-off-by: Ilia Churin <ilia.ch@netogami.net>

ilchu force-pushed the cli-env-vars branch from fe57d6e to b3fe9f3 Compare May 11, 2022 13:37

mversic approved these changes May 11, 2022

View reviewed changes

appetrosyan merged commit 6e3522c into hyperledger:iroha2-dev May 11, 2022

appetrosyan pushed a commit to appetrosyan/iroha that referenced this pull request May 12, 2022

[fix] hyperledger#1990: Enable peer startup via env vars in the absen…

1e2c24d

…ce of config.json (hyperledger#2188) Signed-off-by: Ilia Churin <ilia.ch@netogami.net>

appetrosyan pushed a commit to appetrosyan/iroha that referenced this pull request May 12, 2022

[fix] hyperledger#1990: Enable peer startup via env vars in the absen…

8d985a4

…ce of config.json (hyperledger#2188) Signed-off-by: Ilia Churin <ilia.ch@netogami.net>

appetrosyan pushed a commit to appetrosyan/iroha that referenced this pull request May 12, 2022

[fix] hyperledger#1990: Enable peer startup via env vars in the absen…

b3d2b67

…ce of config.json (hyperledger#2188) Signed-off-by: Ilia Churin <ilia.ch@netogami.net>

appetrosyan pushed a commit to appetrosyan/iroha that referenced this pull request May 12, 2022

[fix] hyperledger#1990: Enable peer startup via env vars in the absen…

9e6de6a

…ce of config.json (hyperledger#2188) Signed-off-by: Ilia Churin <ilia.ch@netogami.net>

ilchu added a commit to appetrosyan/iroha that referenced this pull request May 12, 2022

[fix] hyperledger#1990: Enable peer startup via env vars in the absen…

660f391

…ce of config.json (hyperledger#2188) Signed-off-by: Ilia Churin <churin.ilya@gmail.com>

appetrosyan pushed a commit to appetrosyan/iroha that referenced this pull request May 12, 2022

[fix] hyperledger#1990: Enable peer startup via env vars in the absen…

6de41e4

…ce of config.json (hyperledger#2188) Signed-off-by: Ilia Churin <churin.ilya@gmail.com>

appetrosyan pushed a commit to appetrosyan/iroha that referenced this pull request May 12, 2022

[fix] hyperledger#1990: Enable peer startup via env vars in the absen…

01ca7b7

…ce of config.json (hyperledger#2188) Signed-off-by: Ilia Churin <churin.ilya@gmail.com>

appetrosyan pushed a commit to appetrosyan/iroha that referenced this pull request May 12, 2022

[fix] hyperledger#1990: Enable peer startup via env vars in the absen…

1b5aa41

…ce of config.json (hyperledger#2188) Signed-off-by: Ilia Churin <churin.ilya@gmail.com>

appetrosyan pushed a commit to appetrosyan/iroha that referenced this pull request May 12, 2022

[fix] hyperledger#1990: Enable peer startup via env vars in the absen…

1356928

…ce of config.json (hyperledger#2188) Signed-off-by: Ilia Churin <churin.ilya@gmail.com>

appetrosyan pushed a commit to appetrosyan/iroha that referenced this pull request May 12, 2022

[fix] hyperledger#1990: Enable peer startup via env vars in the absen…

c381ad4

…ce of config.json (hyperledger#2188) Signed-off-by: Ilia Churin <churin.ilya@gmail.com>

appetrosyan pushed a commit to appetrosyan/iroha that referenced this pull request May 12, 2022

[fix] hyperledger#1990: Enable peer startup via env vars in the absen…

8545df4

…ce of config.json (hyperledger#2188) Signed-off-by: Ilia Churin <churin.ilya@gmail.com>

ilchu added a commit to ilchu/iroha that referenced this pull request Aug 2, 2022

[hyperledger#2188]: Work in progress on get_recursive

a749f83

Signed-off-by: Ilia Churin <churin.ilya@gmail.com>

appetrosyan mentioned this pull request Oct 4, 2022

KeyPair defaults #2101

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] #1990: Enable peer startup via env vars in the absence of `config.json` #2188

[fix] #1990: Enable peer startup via env vars in the absence of `config.json` #2188

ilchu commented May 10, 2022

mversic May 10, 2022

ilchu May 10, 2022

mversic May 10, 2022

ilchu May 10, 2022 •

edited

appetrosyan May 10, 2022

codecov bot commented May 11, 2022 •

edited

appetrosyan May 11, 2022

ilchu May 11, 2022

appetrosyan May 11, 2022

[fix] #1990: Enable peer startup via env vars in the absence of config.json #2188

[fix] #1990: Enable peer startup via env vars in the absence of config.json #2188

Conversation

ilchu commented May 10, 2022

Description of the Change

Issue

Benefits

Possible Drawbacks

mversic May 10, 2022

Choose a reason for hiding this comment

ilchu May 10, 2022

Choose a reason for hiding this comment

mversic May 10, 2022

Choose a reason for hiding this comment

ilchu May 10, 2022 • edited

Choose a reason for hiding this comment

appetrosyan May 10, 2022

Choose a reason for hiding this comment

codecov bot commented May 11, 2022 • edited

Codecov Report

appetrosyan May 11, 2022

Choose a reason for hiding this comment

ilchu May 11, 2022

Choose a reason for hiding this comment

appetrosyan May 11, 2022

Choose a reason for hiding this comment

[fix] #1990: Enable peer startup via env vars in the absence of `config.json` #2188

[fix] #1990: Enable peer startup via env vars in the absence of `config.json` #2188

ilchu May 10, 2022 •

edited

codecov bot commented May 11, 2022 •

edited