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

feat: allow custom plugin specification in Docker file and other Docker / plugin improvements #1029

Merged
merged 22 commits into from Sep 15, 2022

Conversation

antazoey
Copy link
Contributor

@antazoey antazoey commented Sep 2, 2022

What I did

fixes: #901
fixes: #456

  • Verify Ape works on Docker build
  • Adds metadata
  • Uses python image instead of Ubuntu
  • Allows specifying plugins to install in Dockerfile

How I did it

  1. Fix all broken plugins
  2. Uninstall rlp before installing (because when already installed as same version, won't re-install and thus never re-generates the correct files).

How to verify it

Checklist

  • All changes are completed
  • New test cases have been added
  • Documentation has been updated

@antazoey antazoey changed the title Fix/jules/docker typing ext fix: docker Sep 2, 2022
fubuloubu
fubuloubu previously approved these changes Sep 2, 2022
Dockerfile Outdated Show resolved Hide resolved
@antazoey antazoey marked this pull request as ready for review September 3, 2022 00:29
fubuloubu
fubuloubu previously approved these changes Sep 3, 2022
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
sabotagebeats
sabotagebeats previously approved these changes Sep 6, 2022
Copy link
Contributor

@sabotagebeats sabotagebeats left a comment

Choose a reason for hiding this comment

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

Thank you for finding this fix!

@antazoey antazoey changed the title fix: docker fix: docker and install plugins from requirements file Sep 7, 2022
@antazoey antazoey marked this pull request as draft September 13, 2022 18:09
@antazoey antazoey changed the title fix: docker and install plugins from requirements file feat: allow custom plugin specification in Docker file and other Docker / plugin improvements Sep 13, 2022
Copy link
Contributor

@sabotagebeats sabotagebeats left a comment

Choose a reason for hiding this comment

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

This is really cool!

@antazoey antazoey marked this pull request as ready for review September 13, 2022 21:46
self.package_name = f"ape-{self.name}" # 'ape-plugin-name'
self.module_name = f"ape_{self.name.replace('-', '_')}" # 'ape_plugin_name'
self.current_version = get_package_version(self.package_name)
class PluginInstallRequest(BaseInterfaceModel):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The refactoring that got done is still really nice I think, even without it's primary drive.


RUN pip install --upgrade pip \
&& pip install --no-cache-dir . \
&& pip install -r recommended-plugins.txt \
Copy link
Contributor Author

@antazoey antazoey Sep 13, 2022

Choose a reason for hiding this comment

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

The recommended plugins get installed via pip -r approach now.

The ape plugins install -r is not needed, as @fubuloubu pointed out that was not a good idea.

I am thinking this may not be the final implementation of this, but it is closer to what we want and achieves the goals.

@@ -0,0 +1,12 @@
ape-alchemy
Copy link
Contributor Author

@antazoey antazoey Sep 13, 2022

Choose a reason for hiding this comment

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

This is shared between Dockerfile and setup.py

Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

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

Not quite a solution to #456, but gets us closer

Might need a docker build workflow for solution to #901

@antazoey
Copy link
Contributor Author

Not quite a solution to #456, but gets us closer

#456 says Allow user to specify which plugins they want in the dockerfile via Dockerfile arguments. The PR adds an ARG to the Dockerfile that allows you to specify plugins to install.. Can you help me understand what is missing in your eyes?

Might need a docker build workflow for solution to #901

Before this PR, there were times where if you ran ape --version (or any command), it would throw a stacktrace. That was why #901 got created. Now, with this PR, the docker build process runs ape --version. If that fails, the image's build will fail and thus it won't publish. Thus, I believe this fixes issue #901 as well.

@fubuloubu
Copy link
Member

fubuloubu commented Sep 14, 2022

Would be nice to have that validation in a PR, if for whatever reason the docker build doesn't work but the tests do

Yes, you are right, that is a good idea. I created a new issue for this here: #1051

@antazoey antazoey enabled auto-merge (squash) September 15, 2022 03:11
@antazoey antazoey merged commit b3dc493 into ApeWorX:main Sep 15, 2022
@antazoey antazoey deleted the fix/jules/docker-typing-ext branch September 20, 2022 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants