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

DO-1920 / allow user specific config #97

Merged
7 commits merged into from
Nov 1, 2023
Merged

DO-1920 / allow user specific config #97

7 commits merged into from
Nov 1, 2023

Conversation

ghost
Copy link

@ghost ghost commented Oct 24, 2023

No description provided.

@github-actions
Copy link

github-actions bot commented Oct 24, 2023

Test Results

53 tests  +3   47 ✔️ +6   20s ⏱️ +2s
  1 suites ±0     6 💤 ±0 
  1 files   ±0     0  - 3 

Results for commit 2c86c95. ± Comparison against base commit 7685a3c.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Oct 24, 2023

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
3846 2573 67% 0% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
node-runner-cli/commands/dockercommand.py 47% 🟢
node-runner-cli/commands/systemdcommand.py 51% 🟢
node-runner-cli/config/SystemDConfig.py 55% 🟢
node-runner-cli/setup/SystemDSetup.py 46% 🟢
node-runner-cli/tests/unit/test_docker.py 97% 🟢
node-runner-cli/tests/unit/test_systemd.py 90% 🟢
TOTAL 64% 🟢

updated for commit: 2c86c95 by action🐍

@ghost ghost marked this pull request as ready for review October 25, 2023 11:18
@ghost ghost requested review from balda-rdx and shambupujar October 25, 2023 11:18
@@ -120,13 +121,19 @@ def config(args):


@dockercommand([
argument("-a", "--autoapprove", help="Pass this option to run without any prompts. "
"Use this for automation purpose only", action="store_true"),
argument("-aue", "--advanceduserenvs",
Copy link
Contributor

Choose a reason for hiding this comment

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

Parameter advanced user doesn't seem right. As this is specific to extra config, probably extranodeconfig or additionalnodeconfig might be better name

@@ -9,6 +9,10 @@ services:
core:
cap_add:
- NET_ADMIN
{% if core_node.advanced_user_envs is defined and core_node.advanced_user_envs is not none %}
env_file:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not mount the file so that it gets picked up here https://github.com/radixdlt/babylon-node/blob/main/docker/build_scripts/config_radixdlt.sh#L15

That way, the additional config doesn't need a environment variable definition at docker level?

Or do you think this is better option and why?

@ghost ghost merged commit 034aa8a into main Nov 1, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants