Skip to content

updated Dockerfile so tests would pass and image would build#167

Open
jeffreybreen wants to merge 5 commits into
databricks:mainfrom
jeffreybreen:master
Open

updated Dockerfile so tests would pass and image would build#167
jeffreybreen wants to merge 5 commits into
databricks:mainfrom
jeffreybreen:master

Conversation

@jeffreybreen
Copy link
Copy Markdown

To follow up on Issue 166, the current Dockerfile fails to build as the tests do not pass.

I have made the following changes and it now builds (and runs the CLI) successfully:

  • The packages listed in tox-requirements.txt are required for the tests to run (and pass), so the Dockerfile now installs them
    • I bumped the versions of several of these packages to resolve errors regarding pytest-django and pyflakes
    • there is still a warning that prospector requires a newer version of pylint, but all tests are now passing
  • click and tabulate packages seem to be required by the main codebase, so I added them to dev-requirements.txt so they are installed (and stick around)
  • In order to satisfy pyroma, the Apache license is now listed in the classifiers in setup.py

To save space in the image:

  • All pip install commands now have the --no-cache-dir option
  • All packages listed in tox-requirements.txt are now removed after the tests have been run
…for pylint-django & pyflakes; still get warning about prospector requiring higher version of pylint, but all tests pass
…so tests will pass; added --no-cache-dir flag to all pip installs
@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #167 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #167   +/-   ##
=======================================
  Coverage   83.38%   83.38%           
=======================================
  Files          30       30           
  Lines        1938     1938           
=======================================
  Hits         1616     1616           
  Misses        322      322
Impacted Files Coverage Δ
setup.py 0% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8637478...3f056a7. Read the comment docs.

Comment thread Dockerfile
COPY . .

RUN pip install --upgrade pip && \
pip install -r dev-requirements.txt && \
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is a minimal fix here just to also install tox-requirements?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That is certainly the heart of the change -- and almost enough to fix the issue,

You also need to bump the versions on some of the packages in tox-requirements.txt or it will fail due to pytest-django and pyflakes dependencies. Also, you need to add the tabulate and click packages to dev-requirements.txt.

My other edits to Dockerfile were an attempt to minimize the resulting image size, but I didn't get a dramatic savings.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Gotcha. I'm a bit concerned about changing the dev-requirements.txt and tox-requirements.txt to make this docker container build work.

I think a minimal fix is to intall tox-requirements.txt and to move up pip install . above the ./lint.sh

Also it may be useful to add a .dockerignore file containing

**/__pycache__
**/*.pyc

so we don't run into https://stackoverflow.com/questions/44067609/getting-error-importmismatcherror-while-running-py-test

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

with @andrewmchen proposals (add tox-requirements to pip install and moving ./lint.sh) works without problem.

thanks

@hagridaaron
Copy link
Copy Markdown

Any idea when this will be merged?

@andrewmchen
Copy link
Copy Markdown
Contributor

@hagridaaron, as it is, this PR isn't ready to merge (merge conflicts and changes to workspace/api.py. Is your goal just to make the docker image build?

@rvvincelli
Copy link
Copy Markdown

On this fork I get:

executor failed running [/bin/sh -c pip install --upgrade --no-cache-dir pip &&       pip install --no-cache-dir             -r dev-requirements.txt            -r tox-requirements.txt &&     pip list &&     ./lint.sh &&     pip install --no-cache-dir . &&     pytest tests  &&     pip uninstall -y         -r tox-requirements.txt]: exit code: 1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

6 participants