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

GitHub Action for DockerHub build and publish #19 #49

Merged
merged 2 commits into from
Oct 3, 2024

Conversation

jsanchez556
Copy link
Contributor

@jsanchez556 jsanchez556 commented Oct 1, 2024

Hi:

Please review this PR. I have attached the GitHub actions logs for reference and included links to the public Docker Hub repository containing the created images.

Action Logs: https://github.com/jsanchez556/stone-packaging/actions/runs/11147558391
DockerHub: https://hub.docker.com/r/blockchainercr/stone-packaging/tags

@jsanchez556 jsanchez556 force-pushed the dockerhub branch 4 times, most recently from 3dace9d to 58d8dd3 Compare October 1, 2024 17:29
Copy link
Member

@dmirgaleev dmirgaleev left a comment

Choose a reason for hiding this comment

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

Good job! May I ask for a couple changes? First, let's fix tags and image names. For your fork following images should be created:
docker pull blockchainercr/stone-prover
docker pull blockchainercr/cpu_air_prover
docker pull blockchainercr/cpu_air_verifier

Also, please, take a look at https://hub.docker.com/r/blockchainercr/stone-packaging/tags. For each image mentioned above, should be created following tags:

  • latest (created when you push something to the master branch)
  • v*.. (created, when you push a tag, for instance v1.0.0)
    If you can't find a way to do that in one single workflow, you can split it in two

I think for Docker Hub we can drop image creation for PRs for now.

@jsanchez556 jsanchez556 force-pushed the dockerhub branch 2 times, most recently from 5db835f to 1986aee Compare October 2, 2024 16:12
@jsanchez556
Copy link
Contributor Author

Good job! May I ask for a couple changes? First, let's fix tags and image names. For your fork following images should be created: docker pull blockchainercr/stone-prover docker pull blockchainercr/cpu_air_prover docker pull blockchainercr/cpu_air_verifier

Also, please, take a look at https://hub.docker.com/r/blockchainercr/stone-packaging/tags. For each image mentioned above, should be created following tags:

  • latest (created when you push something to the master branch)
  • v*.. (created, when you push a tag, for instance v1.0.0)
    If you can't find a way to do that in one single workflow, you can split it in two

I think for Docker Hub we can drop image creation for PRs for now.

@dmirgaleev please check again. ;)

TAG_NAME=$(echo ${GITHUB_REF} | sed 's/refs\/tags\///')
echo "DOCKER_TAGS=${TAG_NAME}" >> $GITHUB_ENV
elif [[ "${GITHUB_REF}" == "refs/heads/master" ]]; then
echo "Latest version tags..."
Copy link
Member

Choose a reason for hiding this comment

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

Great! May I ask you to trigger on-push-master to test it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any problem with doing it on master? Or can I do it on another branch for testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsanchez556 jsanchez556 force-pushed the dockerhub branch 2 times, most recently from 4671e20 to 89a6b58 Compare October 2, 2024 23:29
Copy link
Member

@dmirgaleev dmirgaleev left a comment

Choose a reason for hiding this comment

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

Perfect! Final adjustment:) Also, as an additional task, you can instead of master push use

repository_dispatch:
    types: [prover-update]

But not necessary to do so right now.

build.sh Outdated
Copy link
Member

@dmirgaleev dmirgaleev Oct 3, 2024

Choose a reason for hiding this comment

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

Please, fix permissions for the file, if it's not necessary to have 755, return it back to 644

@jsanchez556
Copy link
Contributor Author

Sure @dmirgaleev, working on it.

@jsanchez556 jsanchez556 force-pushed the dockerhub branch 3 times, most recently from fbb96f8 to 622a829 Compare October 3, 2024 13:37
@dmirgaleev dmirgaleev linked an issue Oct 3, 2024 that may be closed by this pull request
@jsanchez556 jsanchez556 force-pushed the dockerhub branch 2 times, most recently from d5b14b6 to 3bd72a2 Compare October 3, 2024 14:22
Copy link
Member

@dmirgaleev dmirgaleev left a comment

Choose a reason for hiding this comment

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

Good job!

@dmirgaleev dmirgaleev merged commit 0bd3608 into dipdup-io:master Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GitHub Action for DockerHub build and publish
2 participants