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

Docker testing publish #176

Merged
merged 29 commits into from
Jun 21, 2020
Merged

Docker testing publish #176

merged 29 commits into from
Jun 21, 2020

Conversation

BenjaminDev
Copy link
Member

Pull Request

Description

Publish to Docker Hub so it's easy for people to try out the library.

Changes:

  • added a new workflow that builds the image, runs the test suit and (in theory - credentials will be needed) pushes the image to docker hub.

Notes: Dev dependencies are not baked into the image but only used to run tests.

Fixes #163

Affected Dependencies

None

Type of Change

Please mark options that are relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • Documentation (non-breaking change which adds documentation)
  • Improvement (non-breaking change that improves the performance or reliability of existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How has this been tested?

  • Github action is successful.
  • To reporduce: fork and push a change to dev. Note: make sure your github repo settings enables actions to run. Seems my defaults for forks in not to run the actions.

Checklist

Additional Context

Questions to conclude this PR

  • When (which branches) should trigger this action?
  • What tagging convention should we use? (So far just latest and git sha)
  • What image naming should we use?
  • We'll need to add a github secret with docker hub creds allow pushing once the rest has been done.
  • Should the tests we run on these images be against all supported python versions? I am unsure how the current tox setup is working but I'm sure we could run that. Depending on the intended use of these images I guess.

I'm super happy to complete these but someone who knows those answers can pop them in and take over if they would like.

@codeboy5
Copy link
Member

codeboy5 commented Jun 2, 2020

I think we can have the publish workflow to run only a specific branch or tag, and the build needs to run on every PR to check the Docker-file

- name: Run tests
run:
| # Not sure if this is what is desired. If a test fails the whole build fails - can mark flaky tests if needed?
docker run ${{steps.build_name.outputs.image_name}}:latest /bin/bash -c "pipenv install --dev && pipenv run pytest ./tests"
Copy link
Member

Choose a reason for hiding this comment

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

This can be a separate workflow

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 think it would be nicer to run the tests against the docker images before pushing them out the door.
It won't add much to the total time taken to run the action. And the final shipped artifact will be identical.

Copy link
Member

Choose a reason for hiding this comment

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

We want to run docker tests one each commit.
But publishing it to dockerhub is not something we want to do for each commit

@BenjaminDev
Copy link
Member Author

BenjaminDev commented Jun 9, 2020

Remaining items:

  1. Are the image names and tags correct? Currently openmined/pydp-{branch}-{python-version} Is this consistent across the organization?
REPOSITORY                                           TAG                                
openmined/pydp-dev-3.6                               eb3f50ce77c50886051309cdcf7708e7d40ca94a  
openmined/pydp-dev-3.6                               latest   
  1. Un-comment the last stage to push the images to docker hub and add the credentials to github secrets.
  2. Remove verbose comments and scratch marks in Dockerfile. I left it to assist the reviewer.

Notes: I made changes the the Dockerfile as it cloned the thirdparty code which was not pegged to a version or sha. Instead I copy this in by cloning the submodule (same as a local build). l don't see any issues with that but just wanted to mention that incase I have missed something

@BenjaminDev BenjaminDev changed the title [WIP] Docker testing publish Docker testing publish Jun 9, 2020
chinmayshah99
chinmayshah99 previously approved these changes Jun 18, 2020
@chinmayshah99 chinmayshah99 requested a review from codeboy5 June 18, 2020 14:35
It made sense, as people using docker for development already have third-party folder cloned.
@chinmayshah99
Copy link
Member

Hey @BenjaminDev thanks for the PR. Why don't you join us on Slack?

Copy link
Member

@chinmayshah99 chinmayshah99 left a comment

Choose a reason for hiding this comment

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

LGTM!

@chinmayshah99 chinmayshah99 merged commit 883662d into OpenMined:dev Jun 21, 2020
BenjaminDev pushed a commit to BenjaminDev/PyDP that referenced this pull request Jul 3, 2020
changed import

Clone submodules as well so we can peg to a SHA and not break unexpectedly. Include python 3.5 in build images

remove 3rd party from ignore and clone the submodule instead.

Remove cloning 3rd party deps as it was not pegged to a SHA or version. Using git submodule to ensure we pull in the correct version of 3rd code.

explicity stated the pipenv place the venv inside the project dir. Set env vars to  the appropriate virtual env so users can simply run

updated python version arg to upper case

remove debugging

debugging GITHUB_SHA

debugging GITHUB_SHA

removed debugging

added git checkout for sub-mod to specific version

style fix

changed import

fix typo in dockerfile

added checks for python, bazel & pipenv

install bazel using binary installer

switched back

add check pipenv for linux (OpenMined#160)

* add check for pipenv

* Update prereqs_linux.sh

* replace apt with pip

* use python -c to check if pipenv is installed

* use pip3 and python3

* call python instead of python3

* use python3 instead of python

* check pythong using --version

* add elif to double check python

Utils Bindings (OpenMined#180)

* utils bindings

* style fix

* more bindings and tests

* more bindings and tests

* bindings for add,subtract and sqaure

* style fix and TODO added

* throwing runtime error during invalid operations

* removed TODO

Added Docker hub publish (OpenMined#176)

* added github workflow to publish the docker image. Runs test suit as a sanity check.

* Update update-docker-image.yml

* Update update-docker-image.yml

* Update update-docker-image.yml

* Update update-docker-image.yml

* added builds for all python versions.

* syntax

* formatting remove spaces.

* testing

* shuffle the image name concatenation.

* need to references from steps level

* need to references from steps level

* Fix docker args. hyphens are illegal it seems.

* WIP. need to version upstream deps like thet base dp lib from google. Otherwise all our stuff will break unexpectedly. Better to formally pull in deps. However, I see  doesn't have any releases yet.

* Clone submodules as well so we can peg to a SHA and not break unexpectedly. Include python 3.5 in build images

* remove 3rd party from ignore and clone the submodule instead.

* Remove cloning 3rd party deps as it was not pegged to a SHA or version. Using git submodule to ensure we pull in the correct version of 3rd code.

* explicity stated the pipenv place the venv inside the project dir. Set env vars to  the appropriate virtual env so users can simply run

* updated python version arg to upper case

* remove debugging

* debugging GITHUB_SHA

* debugging GITHUB_SHA

* removed debugging

* added docker publish stuff

* added third_party yet again to docker file

It made sense, as people using docker for development already have third-party folder cloned.

* disabled docker hub push on every release

Co-authored-by: Chinmay Shah <[email protected]>

Added error handling (OpenMined#188)

* added bascic exception handling

* added error handling for bounded functions

* added error handling for order statistics

* fixed cpp style

fixing issue OpenMined#92

clang-format

added sample error handling code

0.1.3 release ready

Update update-docker-image.yml

changes in sha version of DP library

docker tagging fix

Update update-docker-image.yml

0.1.3 -> 0.1.4

added docker publish stuff

added third_party yet again to docker file

It made sense, as people using docker for development already have third-party folder cloned.

disabled docker hub push on every release
dvadym pushed a commit to dvadym/PyDP that referenced this pull request Jul 3, 2022
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 actions for Dockerfile
3 participants