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

fix(holesky/docker): add environment variable replacement in entrypoint #371

Merged
merged 6 commits into from
Nov 11, 2024

Conversation

thedevbirb
Copy link
Contributor

@thedevbirb thedevbirb commented Nov 8, 2024

Couple things:

  • as title says, doing so allows actually removing the empty vars as done in fix!(config): remove empty environment variables set like 'VAR=' when parsing options #367. If passed in the environment section or via --env-file flag they are system-wide inside the container and therefore removing them within the sidecar process doesn't work, because you can't remove a system-wide environment variable via env::remove_var.
  • removing empty envs is now done even if a .env file is not provided
  • reading the envs and removing empty ones is done as the first thing in the program, followed by initializing tracing.

@thedevbirb thedevbirb added C: bolt-sidecar Component: bolt-sidecar T: bug Type: Bug labels Nov 8, 2024
Comment on lines 61 to 71
fn init_tracing() -> eyre::Result<()> {
let std_layer = FmtLayer::default().with_writer(std::io::stdout).with_filter(
EnvFilter::builder()
.with_default_directive("bolt_sidecar=info".parse()?)
.from_env_lossy()
.add_directive("reqwest=error".parse()?)
.add_directive("alloy_transport_http=error".parse()?),
);
Registry::default().with(std_layer).try_init()?;
Ok(())
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain why you moved this function here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Imo it was weird that tracing was initialised in a function called init_telemetry_stack, I didn't expect to find it there. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would keep it in telemetry as we use it as a container for "all things monitoring"

@@ -4,23 +4,17 @@ services:
container_name: bolt-sidecar-holesky
restart: unless-stopped
ports:
- "${BOLT_SIDECAR_PORT:-8017}:${BOLT_SIDECAR_PORT:-8017}" # Bolt RPC port (this should be opened on your firewall!)
# NOTE: to read these envs, it is necessary to provide them via `--env-file` or having them already set.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this mean? You removed env_file: ./bolt-sidecar.env and now we need to pass it as a flag when running docker compose up?

If so, why? I don't get what the problem was with leaving the env file there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: it is already like this -- the full command to spin up the docker compose for Holesky is

docker compose --env-file bolt-sidecar.env up -d

See https://github.com/chainbound/bolt/blob/lore%2Ffeat%2Fholesky-launch/testnets/holesky/README.md#L477-L477 (btw docker compose up -d --env-file bolt-sidecar.env is wrong, the argument should be placed before, I'm going to fix that).

That flag is needed because, unless you've them already set, these envs cannot be read in the docker compose file. Lastly, invoking docker compose with the --env-file and then providing the env_file property in the docker compose with the same file is completely redundant.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense, ty!

@merklefruit
Copy link
Collaborator

merklefruit commented Nov 9, 2024

Some more thoughts on this: I don't like the added complexity of this solution vs the benefits that it brings.
Essentially, all of this is a problem because in our example env file we want to include all variables when users might not need all of them, right?

Wouldn't it be possible to just mark optional variables with "OPTIONAL: default true" or "OPTIONAL: default none"?
And then in case of "default: none" we can just comment out the variable itself so it's not parsed.

example:

# URL of the EL node to connect to
# REQUIRED, default: http://localhost:8545
BOLT_SIDECAR_EXECUTION_API_URL="http://localhost:8545"

# If using delegations, this variable must be provided and point to the file
# containing signed delegation messages.
# OPTIONAL, default: none
# BOLT_SIDECAR_DELEGATIONS_FILE=delegations.json

@thedevbirb
Copy link
Contributor Author

Some more thoughts on this: I don't like the added complexity of this solution vs the benefits that it brings. Essentially, all of this is a problem because in our example env file we want to include all variables when users might not need all of them, right?

Wouldn't it be possible to just mark optional variables with "OPTIONAL: default true" or "OPTIONAL: default none"? And then in case of "default: none" we can just comment out the variable itself so it's not parsed.

example:

# URL of the EL node to connect to
# REQUIRED, default: http://localhost:8545
BOLT_SIDECAR_EXECUTION_API_URL="http://localhost:8545"

# If using delegations, this variable must be provided and point to the file
# containing signed delegation messages.
# OPTIONAL, default: none
# BOLT_SIDECAR_DELEGATIONS_FILE=delegations.json

We actually do it for two reasons:

  • the first one is what you mentioned i.e., removing empty environment variables so that they can be set like VAR=; in our context I think it is a good safety measure since we don't have environment variables that makes sense with an empty string value.
  • The other is setting environment variables conditionally, something that is not possible in docker compose in the environment section. Note that it is a but is a system wide effect in the container so if you set them empty the sidecar software cannot truly unset them inside its process. For this reason I've introduced the entrypoint.sh file that allows us to do whatever overriding logic we need, and by re-exporting the environment variables in the same process that will spin up the sidecar we don't incur in the issue above mentioned.

All of this is done improve UX on the user side: just write down your preferences in the bolt-sidecar.env file without the need of modifying the Docker compose file (mounting extra volumes etc) according to your configuration.

@merklefruit
Copy link
Collaborator

Thanks for the detailed explanation, sounds good!

@thedevbirb thedevbirb merged commit e2bc4e1 into lore/feat/holesky-launch Nov 11, 2024
3 checks passed
@thedevbirb thedevbirb deleted the lore/fix/docker-again branch November 11, 2024 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: bolt-sidecar Component: bolt-sidecar T: bug Type: Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants