Skip to content

Add no cache and Optimize the Dockerfile format#1451

Merged
asfgit merged 1 commit intoapache:trunkfrom
99Kies:trunk
May 3, 2020
Merged

Add no cache and Optimize the Dockerfile format#1451
asfgit merged 1 commit intoapache:trunkfrom
99Kies:trunk

Conversation

@99Kies
Copy link
Copy Markdown
Contributor

@99Kies 99Kies commented Apr 30, 2020

Changes Title (replace this with a logical title for your changes)

Description

Replace this with the PR description (mention the changes you have made, why
you have made them, provide some background and any references to the provider
documentation if needed, etc.).

For more information on contributing, please see Contributing
section of our documentation.

Clean up the Dockerfile code format and remove the cache residue after pip install

Status

Replace this: describe the PR status. Examples:

  • work in progress
  • done, ready for review

done

Checklist (tick everything that applies)

  • Code linting (required, can be done after the PR checks)
  • Documentation
  • Tests
  • ICLA (required for bigger changes)

@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #1451 into trunk will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##            trunk    #1451   +/-   ##
=======================================
  Coverage   86.15%   86.15%           
=======================================
  Files         375      375           
  Lines       78630    78630           
  Branches     7770     7770           
=======================================
  Hits        67743    67743           
  Misses       7953     7953           
  Partials     2934     2934           

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 5b54d72...31bce07. Read the comment docs.

@Kami
Copy link
Copy Markdown
Member

Kami commented May 3, 2020

Thanks for the contribution.

I'll test it locally and ensure that it's working as expected.

Besides that - we recently dropped support for Python < 3.4 so we can also remove those Python versions from Dockerfile.

WORKDIR /libcloud
CMD tox -e py2.7,pypypy,py3.4,py3.5,py3.6,lint

CMD ["tox", "-e", "py2.7", "pypypy", "py3.4", "py3.5", "py3.6", "lint"]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As per my comment on the PR - once I confirm it's working, I will go ahead and split into two Dockerfiles - Dockerfile.python3 and Dockerfile.python2.

In the future, we can also get rid of python2 one all together since we don't officially support it anymore anyway.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, Upgrade to python3 is definitely necessary.

@Kami
Copy link
Copy Markdown
Member

Kami commented May 3, 2020

I receive the following error when I try to build a fresh Docker image:

  File "/usr/local/lib/python2.7/dist-packages/importlib_metadata/__init__.py", line 9, in <module>
    import zipp
ImportError: No module named zipp

It appears to be related to old version of pip - I will look into upgrading it as part of the Dockerfile step.

WORKDIR /libcloud
CMD tox -e py2.7,pypypy,py3.4,py3.5,py3.6,lint

CMD ["tox", "-e", "py2.7", "pypypy", "py3.4", "py3.5", "py3.6", "lint"]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This actually won't work correctly, targets need to be passed to tox as a single argument.

So either:

tox -epy1,py2...

Or:

["tox", "-e", "py1,py"]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i got it.

Copy link
Copy Markdown
Contributor Author

@99Kies 99Kies May 3, 2020

Choose a reason for hiding this comment

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

This is my fault, I haven’t tested it because of time.

@Kami
Copy link
Copy Markdown
Member

Kami commented May 3, 2020

Thanks for the original contribution, but after trying to get it to work locally it turned out it needs much more work.

It was likely broken and not working for quite a while now.

Here are the changes I needed to make to get it in sync with latest master and everything passing - Kami@bf25ca3, Kami@e2a5cbc, Kami@a3eb0a2.

I will also see if I can get it to run on our CI - if we don't do that, it's better to remove it all together from this repo. Otherwise will just get out of date and broken again sooner or later.

And in the future, we also need to update base image to be something more recent than Ubuntu 16.04 which defaults to Python 3.

@Kami
Copy link
Copy Markdown
Member

Kami commented May 3, 2020

Here we go - Kami@ca9c274.

I also pushed a change to use newer Ubuntu version and Python 3 for running tox target. Now it's more or less in sync with latest master / trunk and we just need to sort running it on CI.

@Kami
Copy link
Copy Markdown
Member

Kami commented May 3, 2020

Alright, took a couple of more fixes (Kami@c2edeeb, Kami@15c1ffe), but it's now passing and also hooked up to CI - https://travis-ci.org/github/Kami/libcloud/jobs/682629304.

Only downside is that this CI target is quite slow (8 -10 minutes). But as mentioned above, not having it hooked up to CI is worse than not having this Dockerfile at all :)

@asfgit asfgit merged commit 31bce07 into apache:trunk May 3, 2020
@99Kies
Copy link
Copy Markdown
Contributor Author

99Kies commented May 3, 2020

Alright, took a couple of more fixes (Kami@c2edeeb, Kami@15c1ffe), but it's now passing and also hooked up to CI - https://travis-ci.org/github/Kami/libcloud/jobs/682629304.

Only downside is that this CI target is quite slow (8 -10 minutes). But as mentioned above, not having it hooked up to CI is worse than not having this Dockerfile at all :)

nice, :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants