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

feat(release): create Docker hub binary with mining enabled on release #6228

Merged
merged 9 commits into from
Mar 24, 2023

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Feb 24, 2023

Motivation

We want to make it easy for miners to test Zebra's getblocktemplate RPC methods.

Closes #6200.

Solution

  • Add a Dockerfile with:
    • getblocktemplate-rpcs feature enabled
    • Network=Testnet by default
    • An exposed RPC listener port
  • Add a workflow for building and publishing images on 'release'

Review

Anyone can review.

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?

@arya2 arya2 added A-devops Area: Pipelines, CI/CD and Dockerfiles C-enhancement Category: This is an improvement P-Medium ⚡ A-rpc Area: Remote Procedure Call interfaces labels Feb 24, 2023
@arya2 arya2 self-assigned this Feb 24, 2023
@arya2 arya2 requested a review from a team as a code owner February 24, 2023 21:46
@arya2 arya2 requested review from teor2345 and removed request for a team February 24, 2023 21:46
@github-actions github-actions bot added C-feature Category: New features C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Feb 24, 2023
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.

I'm concerned that we are creating a duplicate Dockerfile, it will be much harder to maintain than the single test/production Dockerfile we have at the moment.

Can we make the mining RPCs depend on a Docker argument instead?

docker/mining.Dockerfile Outdated Show resolved Hide resolved
docker/mining.Dockerfile Outdated Show resolved Hide resolved
docker/mining.Dockerfile Outdated Show resolved Hide resolved
@teor2345
Copy link
Contributor

teor2345 commented Mar 1, 2023

This PR will need a rebase on the latest main to pick up some new CI jobs.

Copy link
Member

@gustavovalverde gustavovalverde left a comment

Choose a reason for hiding this comment

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

I wasn't able to see this PR previously, but I do agree we need to make this happen on our existing Dockerfile.

@arya2 let me know if you'd like some help with this changes.

@arya2 arya2 requested a review from teor2345 March 21, 2023 20:48
docker/Dockerfile Outdated Show resolved Hide resolved
@teor2345 teor2345 dismissed stale reviews from gustavovalverde and themself March 22, 2023 04:51

Duplicate files have been resolved

@mpguerra mpguerra requested review from teor2345 and removed request for gustavovalverde March 23, 2023 09:29
arya2 and others added 2 commits March 23, 2023 20:32
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.

This looks good, but I think we need to test it by making a fake .test release. workflow_dispatch won't work on this workflow, so we'll need to do the test after merging to main.

Here's the kinds of things I want to check:

  1. Is the test or production image the first image that users see?
  2. If users are pulling down the latest image, do they always get the production image?
  3. Is the test image clearly marked as experimental?

If it could be confusing, our other option is to:

  1. Use a different project name like zfnd/zebra-experimental
  2. Remove any images we've pushed to the zfnd/zebra DockerHub

@teor2345
Copy link
Contributor

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Mar 24, 2023

refresh

✅ Pull request refreshed

mergify bot added a commit that referenced this pull request Mar 24, 2023
@mergify mergify bot merged commit 671420b into main Mar 24, 2023
@mergify mergify bot deleted the testnet-mining-docker branch March 24, 2023 07:10
@teor2345
Copy link
Contributor

This looks good, but I think we need to test it by making a fake .test release. workflow_dispatch won't work on this workflow, so we'll need to do the test after merging to main.

Here's the kinds of things I want to check:

1. Is the test or production image the first image that users see?

2. If users are pulling down the `latest` image, do they always get the production image?

3. Is the test image clearly marked as experimental?

If it could be confusing, our other option is to:

1. Use a different project name like `zfnd/zebra-experimental`

2. Remove any images we've pushed to the `zfnd/zebra` DockerHub

@mpguerra @arya2 when do you think we should do this testing? Is it ok to wait until the next release and just see what happens?

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 A-rpc Area: Remote Procedure Call interfaces C-enhancement Category: This is an improvement C-feature Category: New features C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Publish Docker image with mining enabled
3 participants