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

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jeffreybreen
Copy link

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

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.

@@ -4,11 +4,15 @@ WORKDIR /usr/src/databricks-cli

COPY . .

RUN pip install --upgrade pip && \
pip install -r dev-requirements.txt && \
Copy link
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
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
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

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

Any idea when this will be merged?

@andrewmchen
Copy link
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

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants