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(Docker): Set default RPC_PORT #7162

Merged
merged 1 commit into from Jul 6, 2023
Merged

Conversation

upbqdn
Copy link
Member

@upbqdn upbqdn commented Jul 6, 2023

Motivation

I noticed we were using an unset RPC_PORT in the runtime entry point.

Solution

The runtime-entrypoint.sh uses the RPC_PORT env var when the user
specifies the getblocktemplate-rpc feature, but this env var is unset
unless the user sets it. This commit sets the default values for
RPC_PORT depending on NETWORK.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

@upbqdn upbqdn added C-bug Category: This is a bug A-devops Area: Pipelines, CI/CD and Dockerfiles P-High 🔥 labels Jul 6, 2023
@upbqdn upbqdn requested a review from a team as a code owner July 6, 2023 12:12
@upbqdn upbqdn self-assigned this Jul 6, 2023
@upbqdn upbqdn requested review from teor2345 and removed request for a team July 6, 2023 12:12
arya2
arya2 previously approved these changes Jul 6, 2023
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

Good catch!

docker/Dockerfile Outdated Show resolved Hide resolved
docker/runtime-entrypoint.sh Show resolved Hide resolved
@arya2 arya2 dismissed their stale review July 6, 2023 17:45

Let's make sure it works with --target runtime too.

The `runtime-entrypoint.sh` uses the `RPC_PORT` env var when the user
specifies the `getblocktemplate-rpc` feature, but this env var is unset
unless the user sets it. This commit sets the default values for
`RPC_PORT` depending on `NETWORK`.
@upbqdn upbqdn force-pushed the fix-dockerfile-entrypoint branch from f50331b to a81c309 Compare July 6, 2023 17:56
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

Thank you!

@teor2345 teor2345 removed their request for review July 6, 2023 19:06
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

The runtime-entrypoint.sh uses the RPC_PORT env var when the user
specifies the getblocktemplate-rpc feature

Some users might want to use the Docker image with lightwalletd, but without configuring getblocktemplate-rpcs. So let's work out how to do that in another ticket.

Previously, the Dockerfile only activated the RPC port when it was set, but I agree that doesn't make sense with getblocktemplate-rpcs:
8c90f65#diff-5b838857fc3410c2438196effdbdcffae6548bdba431a4a938e76a0be62d008eL49

So we might want logic like:

  • If getblocktemplate-rpcs is activated, activate the default RPC port if it is unset
  • If there aren't any active RPC features, activate the RPC port only if it is set
  • Otherwise, don't activate the RPC port

This covers the 3 different use cases of mining, light wallet, and no RPCs.

mergify bot added a commit that referenced this pull request Jul 6, 2023
@mergify mergify bot merged commit 1014e3c into main Jul 6, 2023
277 checks passed
@mergify mergify bot deleted the fix-dockerfile-entrypoint branch July 6, 2023 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles C-bug Category: This is a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants