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

Publish Docker Image To GitHub Container Registry (ghcr.io) #5118

Merged
merged 16 commits into from
Apr 13, 2023

Conversation

hamelsmu
Copy link
Contributor

@hamelsmu hamelsmu commented Apr 7, 2023

Adding automation that I discussed with @jjallaire

To make this work, we will have to:

  1. Create an official posit account on DockerHub, and create an empty repo named quarto
  2. Store the secret token in secrets.DOCKER_TOKEN in this repo's secrets

I don't know if this is what you want, but I figured a PR would help out to move it closer to the finish line!

@cderv
Copy link
Collaborator

cderv commented Apr 7, 2023

Chiming to share some thoughts.

As I don't think there is an official posit account on dockerhub and everything is still at https://hub.docker.com/u/rstudio. Also I don't know the plan but it also has been taken already by a posit docker user

One option we could consider is also to store the containers with the quarto-dev organisation directly. Github offers Github Package product and this is available for Open Source project like us.

It would live at https://github.com/orgs/quarto-dev/packages and I think available at

docker pull ghcr.io/quarto-dev/quarto

(doc about container registry feature)

Could be a good option IMO to keep everything in here.

@jjallaire
Copy link
Collaborator

@cderv Yes I think using the GitHub Package product with the quarto-dev org sounds perfect here!

Could you work with @hamelsmu on getting this PR aligned to use that method? (then just merge when ready)

@hamelsmu
Copy link
Contributor Author

hamelsmu commented Apr 7, 2023

I'm more than happy to make the change to point to GitHub package registry. Let me try and test the changes locally and I'll update this PR.

Great suggestion by the way ! The GitHub Package Registry has come a long way 😀

@cderv
Copy link
Collaborator

cderv commented Apr 7, 2023

@jjallaire yes sure ! We'll make that happen.

@hamelsmu I believe we can use the GITHUB_TOKEN to push to the registry. Hopefully it will have enough permissions. Though I don't think it will work from a PR branch coming from a fork. So I may need to take your branch and create a new one to make the Github actions works to publish. (this is a thing with access give to GITHUB_TOKEN in external vs internal PR.

I need to go over the docs at https://docs.github.com/en/packages/managing-github-packages-using-github-actions-workflows/publishing-and-installing-a-package-with-github-actions to check that we are correctly setup.

Once you've tested things locally, I can probably take over to integrate in our workflows and repo.

@hamelsmu
Copy link
Contributor Author

hamelsmu commented Apr 7, 2023

@cderv Yes I figured it out! I updated this PR.

The trick is to modify the token's permissions with this setting

permissions:
    packages: write

I've tested this Action on my username so hopefully it works the first time!

@hamelsmu hamelsmu changed the title Publish Docker Image To DockerHub Publish Docker Image To GitHub Container Registry (ghcr.io) Apr 7, 2023
Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

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

General comment in addition to in file comments. This is some thoughts about integration on your work in the overall project.

I have one concerns: I just want to prevent the publish step to fail because of docker build failure for some reason, or publishing to registry error.

Now that I can see how this works thanks to your work. I see several solutions

  • we could add for now continue-on-error on the docker step.
  • do another job for this using needs to depend on the linux one. The workflow would fail but only because of this jobs. The release would still be created.
  • Last options I see is to have this as another workflow to be triggered after the create-release one has been done. This other workflow would get the file required from the release, and then published to the registry.

This is some thoughts also for later as I'll do a pass on all actions for 1.4 so I can improve at this point.

I can take over and do some modification before merging if you want.

.github/workflows/create-release.yml Outdated Show resolved Hide resolved
docker/action.yml Outdated Show resolved Hide resolved
.github/workflows/create-release.yml Outdated Show resolved Hide resolved
docker/action.yml Outdated Show resolved Hide resolved
docker/action.yml Outdated Show resolved Hide resolved
@hamelsmu
Copy link
Contributor Author

hamelsmu commented Apr 7, 2023

@cderv I went with this suggestion (which I think is very reasonable)

do another job for this using needs to depend on the linux one. The workflow would fail but only because of this jobs. The release would still be created.

@eitsupi
Copy link
Contributor

eitsupi commented Apr 9, 2023

Some questions.

  • Why use Ubuntu instead of Debian? In my opinion Debian has the advantage of easy Chromium installation and may be more suitable for Quarto.
  • Is there any reason not to do a multi-platform build (amd64 and arm64) here? It would not be difficult to do that as the deb package for arm64 is also currently built every time.

@hamelsmu
Copy link
Contributor Author

hamelsmu commented Apr 9, 2023

Why use Ubuntu instead of Debian

I've found that data science-y type of things have the best compatibility with Ubuntu. Feel free to change it if you like!

Is there any reason not to do a multi-platform build (amd64 and arm64) here? It would not be difficult to do that as the deb package for arm64 is also currently built every time.

We could certainly do this. I was trying to scope it to be simple initially but we could expand it to build on all platforms that you build packages for.

@cderv
Copy link
Collaborator

cderv commented Apr 12, 2023

@eitsupi this is a first proposal based on a contribution. We'll make adaptation over time.

We need to see how to position also regarding community project like #5129 @hamelsmu did you see that already ?

They are already in an advance stage at https://github.com/jdutant/quarto-dockerfiles and published at https://hub.docker.com/r/jdutant/quarto-minimal

They are also on Ubuntu and for both architectures

@hamelsmu
Copy link
Contributor Author

hamelsmu commented Apr 12, 2023

@cderv Let me know if I should change anything about this PR (wasn't sure so wanted to check)

@cderv
Copy link
Collaborator

cderv commented Apr 12, 2023

@hamelsmu I am currently testing this in my fork to check everything works as expected. A few adjustment to make. I'll comment in a review and do the change probably. Thanks !

Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

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

Here is what I noticed we need to change for this to work effectively in the repo. I'll push the change in the branch directly

.github/workflows/create-release.yml Outdated Show resolved Hide resolved
.github/actions/docker/action.yml Show resolved Hide resolved
.github/workflows/create-release.yml Show resolved Hide resolved
.github/actions/docker/action.yml Outdated Show resolved Hide resolved
setting permissions with YAML on workflow will unset any set by default.
Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

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

I think this is all good now. Everything is working fine and container are published in github Package as expected. Release are still created as we expect.

I'll share with the team internally in our meeting, and then I'll merge when possible (just taking precaution as we are finalizing the 1.3 release, and things are frozen more than usual because of that).

Thanks a lot @hamelsmu !

@hamelsmu
Copy link
Contributor Author

Excellent! Sorry again for the bumpy PR. I've always struggled with testing GHA on a fork and thought I could get away with a small subset of the flow. Next time I'll keep this in mind and recreate the entire flow end to end. 🙇🏽

@cderv
Copy link
Collaborator

cderv commented Apr 13, 2023

Really no worry. For such change that impact a workflow as our create release workflow, we would want to test it correctly on our own anyway. It is really hard to test infrastructure changes from PR anyway.

I am getting used to that now, but it requires working from a fork by activating actions and all, and also doing some change to make sure that the action running won't have some effect of the main repo. I had bad surprised in the past about that because as a user member of the main repo I also have access and can do stuff. If you do that from your fork, there would be less issue because you can push, create release or else on the main repo.

Anyway, really no worry. It was really interesting for me and gave me idea for evolution (with new image less minimal for example).

Merging now !

Thanks !

@cderv cderv merged commit 90707f4 into quarto-dev:main Apr 13, 2023
@cderv
Copy link
Collaborator

cderv commented Apr 14, 2023

@hamelsmu first one is available here: https://github.com/quarto-dev/quarto-cli/pkgs/container/quarto

it should be visible publicly now. We'll monitor the next runs to see if this is working as expected

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.

4 participants