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.

Sign up for GitHub

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

Jump to bottom

docs: add docker compose example #501

Merged
merged 5 commits into from
Nov 16, 2022
Merged

docs: add docker compose example #501

merged 5 commits into from
Nov 16, 2022

Conversation

b5
Copy link
Member

@b5 b5 commented Nov 16, 2022

and fix env var setting in the process. requires #500

Seeing this work is nice 😄. closes n0-computer/beetle#108

@b5 b5 added this to the v0.1.1 milestone Nov 16, 2022
@b5 b5 requested a review from Arqu November 16, 2022 04:12
@b5 b5 self-assigned this Nov 16, 2022
@b5 b5 marked this pull request as ready for review November 16, 2022 04:12
Arqu
Arqu approved these changes Nov 16, 2022
View reviewed changes
Copy link
Collaborator

@Arqu Arqu left a comment

Choose a reason for hiding this comment

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

LGTM, make sure to rebase after the rpc merge.

Arqu
Arqu requested changes Nov 16, 2022
View reviewed changes
Copy link
Collaborator

@Arqu Arqu left a comment

Choose a reason for hiding this comment

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

Actually fix tests first 🤣

@b5
fix: use '__' as env var field separator
69b54ec 
pathing into Config.field.sub_field now works via CONFIG__FIELD__SUB_FIELD
using double underbar de-conflicts with space representation. It's not pretty,
but it's the least terrible thing I've seen.

Shoutout @Frando for the fix
@b5
docs: add docker compose example
2e8d0be
@b5
add docker push xtask
2257182
@b5
use '__' for IROH_METRICS as well
ca4a7c3
@b5 b5 requested a review from Arqu November 16, 2022 14:26
@b5
Copy link
Member Author

b5 commented Nov 16, 2022

@Arqu, hey look at that, the tests caught an issue. It also highlighted an inconsistency that might mess with your life in DevOps land: this PR adjusts IROH_METRICS to also use __ for pathing into compound types, so IROH_METRICS_FOO becomes IROH_METRICS__FOO.

I think we need to change this to stay consistent. Exceptions remain for IROH_ENV, and IROH_INSTANCE_ID, which are unchanged by this PR.

@Arqu
Copy link
Collaborator

Arqu commented Nov 16, 2022

@Arqu, hey look at that, the tests caught an issue. It also highlighted an inconsistency that might mess with your life in DevOps land: this PR adjusts IROH_METRICS to also use __ for pathing into compound types, so IROH_METRICS_FOO becomes IROH_METRICS__FOO.

I think we need to change this to stay consistent. Exceptions remain for IROH_ENV, and IROH_INSTANCE_ID, which are unchanged by this PR.

Good point, feel free to run with this, I'll clean up on my end.

Arqu
Arqu approved these changes Nov 16, 2022
View reviewed changes
Copy link
Collaborator

@Arqu Arqu left a comment

Choose a reason for hiding this comment

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

LGTM if CI passes

Arqu
Arqu approved these changes Nov 16, 2022
View reviewed changes
Copy link
Collaborator

@Arqu Arqu left a comment

Choose a reason for hiding this comment

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

🐹

@b5 b5 merged commit 579c8ed into main Nov 16, 2022
@b5
Copy link
Member Author

b5 commented Nov 16, 2022

cc @Frando, this PR merged your suggested __ path separator for env vars. Thanks so much for the suggestion!

@Arqu Arqu deleted the b5/docker_compose branch November 17, 2022 08:57
@b5 b5 mentioned this pull request Nov 18, 2022
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Arqu Arqu Arqu approved these changes

Assignees

@b5 b5

Labels
None yet
Projects
None yet
Milestone
v0.1.1
Development

Successfully merging this pull request may close these issues.

docker mvp
2 participants
@b5 @Arqu