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

Action repo2docker #29

Closed
wants to merge 5 commits into from
Closed

Action repo2docker #29

wants to merge 5 commits into from

Conversation

jburel
Copy link
Member

@jburel jburel commented Feb 3, 2021

This PR uses https://github.com/jupyterhub/repo2docker-action actions to build the repository
The image is cached on dockerhub when the repository is tagged. We could opt to do it when a PR is merged to master

I have tested the push to dockerhub see
https://hub.docker.com/r/jburel/omero-guide-python/tags

See
https://github.com/jburel/omero-guide-python/actions/runs/534090543 and
https://github.com/jburel/omero-guide-python/actions/runs/534129704

When we are happy if the implementation, I will update https://github.com/ome/.github/blob/master/workflow-templates/repo2docker.yml
and the workflows in the various repositories using the template

Secrets will need to be added for the deployment to dockerhub.

@github-actions
Copy link

github-actions bot commented Feb 3, 2021

Binder 👈 Launch a binder notebook on branch action_repo2docker

run: pip install --upgrade jupyter-repo2docker
- name: Build container
run: repo2docker --no-run .
- name: Set no_push variable
Copy link
Member

Choose a reason for hiding this comment

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

Wonder if just using

run: repo2docker --no-run .
if: no-tag
uses: jupyterhub/repo2docker-action@master
with:
if: tag

wouldn't be more maintainable.

Copy link
Member Author

@jburel jburel Feb 4, 2021

Choose a reason for hiding this comment

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

This will mean installing jupyter-repo2docker manually (as we had before)
or using the docker container (with repo2docker) installed by jupyterhub/repo2docker-action. I am not convinced it will simplify things. In this case we only use one approach allowing to build or build and push

The bulk of the work is to determine the push parameter.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking mainly in terms of total numbers of lines. If your new block is only run on tags, then it's just one extra line, right?

Copy link
Member Author

@jburel jburel Feb 4, 2021

Choose a reason for hiding this comment

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

what will need to be added is

- name: Get the version
        id: get_version
        run: echo ::set-output name=VERSION::${GITHUB_REF#refs/tags/}
      - name: update jupyter dependencies with repo2docker
        uses: jupyterhub/repo2docker-action@master
        if: startsWith(github.ref, 'refs/tags')
        with:
          DOCKER_USERNAME: ${{ secrets.DOCKER_USERNAME }}
          DOCKER_PASSWORD: ${{ secrets.DOCKER_PASSWORD }}
          PUBLIC_REGISTRY_CHECK: true
          ADDITIONAL_TAG: ${{ steps.get_version.outputs.VERSION }}
          IMAGE_NAME: ${{ github.repository }}

or

- name: update jupyter dependencies with repo2docker
        uses: jupyterhub/repo2docker-action@master
        if: startsWith(github.ref, 'refs/tags')
        with:
          DOCKER_USERNAME: ${{ secrets.DOCKER_USERNAME }}
          DOCKER_PASSWORD: ${{ secrets.DOCKER_PASSWORD }}
          PUBLIC_REGISTRY_CHECK: true
          IMAGE_NAME: ${{ github.repository }}

if we want to have the commit sha cf. https://hub.docker.com/layers/135963980/jburel/omero-guide-python/daa95a38d4bf/images/sha256-fe81ab02ded8300a117054668597deddde6f6cd181230f25d5ee4c0b5101a977?context=explore
(I don't think that is a good option if we push on tag)

that's why I am not sold on the fact that it will make things more maintainable

Copy link
Member Author

Choose a reason for hiding this comment

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

I could remove

      - name: Check no no_push_to_cache
        run: echo ${{ steps.nopush.outputs.nopush }}

since it is only there to be sure it is set correctly

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

@sbesson
Copy link
Member

sbesson commented Feb 15, 2021

Overall

  • 👍 for adding logic to push images to the registry
  • 👍 for using the upstream jupyterhub action

Re the discussion started in #29 (comment), I personally find the GitHub actions run steps hard to read as a general rule. Trying to bring a third alternative on the table, has it be considered to use jupyterhub/repo2docker-action@master action unconditionally so that every push event is built and pushed to the Docker registry and have the additional logic on tags to set ADDITIONAL_TAG?

@jburel
Copy link
Member Author

jburel commented Feb 15, 2021

That's what I indicated in the description i.e. we could push on every merge in that case the reference will be the commit
It will be something https://hub.docker.com/layers/jburel/omero-guide-python/a8c10c94cbc8/images/sha256-0ce815e65b77acca3f2a2d4b3bec61a83783b3c58508ade746e858c601c17c8f?context=explore

@sbesson
Copy link
Member

sbesson commented Feb 15, 2021

So the main downside of this approach is the growing number of Dockerg image tags i.e. one tag named after the Git commit SHA for each push plus optionally x.y.z per GitHub tag, is that correct?

@jburel
Copy link
Member Author

jburel commented Feb 15, 2021

It is, and we will also have new image tag with no changes in the docker image itself since we will have PR fixing the doc for example.

@joshmoore
Copy link
Member

Would an alternative be to use the PR number?

@jburel
Copy link
Member Author

jburel commented Feb 16, 2021

The name can be set instead of the commit sha so the PR number could be used that will not be a problem.
It is more the fact that we will potentially push "new" images that are not really new.
A counter argument to that is that infra like VAE will only install the tagged image and not the latest pushed image

@snoopycrimecop
Copy link
Member

Conflicting PR. Removed from build OMERO-guides-push#352. See the console output for more details.
Possible conflicts:

  • Upstream changes
    • .github/workflows/repo2docker.yml

--conflicts

@khaledk2
Copy link

Conflicting PR. Removed from build OMERO-guides-push#1. See the console output for more details.
Possible conflicts:

  • Upstream changes
    • .github/workflows/repo2docker.yml

--conflicts

@snoopycrimecop
Copy link
Member

Conflicting PR. Removed from build OMERO-guides-push#903. See the console output for more details.
Possible conflicts:

  • Upstream changes
    • .github/workflows/repo2docker.yml

--conflicts

@snoopycrimecop
Copy link
Member

Conflicting PR. Removed from build OMERO-guides-push#146. See the console output for more details.
Possible conflicts:

  • Upstream changes
    • .github/workflows/repo2docker.yml

--conflicts

@jburel jburel closed this Apr 28, 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.

None yet

5 participants