adds Docker build on PR - Githubissues

OpenMined / PyDP

The Python Differential Privacy Library. Built on top of: https://github.com/google/differential-privacy

Apache License 2.0

507 stars 138 forks source link

adds Docker build on PR #267

Closed Vishwaak closed 4 years ago

Vishwaak commented 4 years ago

Description

Closes #260, As requested by @chinmayshah99 every PR to dev branch triggers a docker build to check if the PR breaks the docker image or not.

Affected Dependencies

black version is hardcoded to 19.10b0

How has this been tested?

Checked on test PR

Checklist

[x] I have followed the Contribution Guidelines and Code of Conduct
[ ] I have commented my code following the OpenMined Styleguide
[ ] I have labeled this PR with the relevant Type labels
[ ] My changes are covered by tests

chinmayshah99 commented 4 years ago

@BenjaminDev @alejandrosame The idea of #260 and this PR is to avoid issues during release as faced here: https://github.com/OpenMined/PyDP/runs/996868032?check_suite_focus=true

alejandrosame commented 4 years ago

@chinmayshah99 If we are publishing official images, then it is even more relevant to test that the built container passes the tests. For example, it is possible to change the Dockerfile in a way that builds without complaining but fails at execution. The more extreme case is by simply not adding the package, but it could be due to more subtle changes like ENV variables.

This PR can move on as it is, but I do see the value on testing the container built!

chinmayshah99 commented 4 years ago

This PR can move on as it is, but I do see the value on testing the container built!

Just to confirm, testing each container for all PRs?

alejandrosame commented 4 years ago

I think it has to be for PRs that aim to be published, so this maybe only applies to specific branches. Eventually, we should aim to cover as much as it's possible anything that is going to be published.

chinmayshah99 commented 4 years ago

In that case, feel free to create another issue. Let's merge this PR in the meantime.

Regards

Chinmay Shah

On Wed, Aug 26, 2020, 14:44 Alejandro Sánchez Medina < notifications@github.com> wrote:

I think so, given that it seems that there will be an image per Python version. Eventually, we should aim to cover as much as it's possible anything that is going to be published.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/OpenMined/PyDP/pull/267#issuecomment-680760866, or unsubscribe

.

chinmayshah99 commented 4 years ago

hi @Vishwaak, there seems like a mismatch between the python versions. In one step, we are saying we are using python 38, but then we are using py37 underneath the hood. More on it here: https://github.com/OpenMined/PyDP/pull/278/checks?check_run_id=1042574486#step:5:3 Can this typo be fixed?

Vishwaak commented 4 years ago

Hello, @chinmayshah99 that arg value sets the default value of the python version, and uses it if not specified during run time. But it's not the case here.

chinmayshah99 commented 4 years ago

In that case, just mention in the job name that we are using py37.

Vishwaak commented 4 years ago

Wouldn't that contradict the fact that we are using py3.8, to build the image?