-
-
Notifications
You must be signed in to change notification settings - Fork 44
Separate and simplify PyDis specific usage #248
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
base: main
Are you sure you want to change the base?
Conversation
558c6b3 to
77685ac
Compare
05a0526 to
f118990
Compare
Our usage of store_const with positionals is now unsupported and gives a ValueError on 3.14, this change maintains the same behaviour (splitting based on --- to fill nsjail_args and py_args)
73a4cd0 to
d08f0bc
Compare
| services: | ||
| registry: | ||
| image: registry:2 | ||
| ports: | ||
| - 5000:5000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of hosting a registry in our job?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a registry allows us to use the newly built ghcr.io/python-discord/snekbox image for our snekbox-pydis build (you will see later that images are pushed to localhost:5000).
https://stackoverflow.com/a/63927832
There is probably some combination of cache-to/cache-from that may also work here but creating a local registry is a 3 second operation that then ensure we are using the right images and there will be no attempt made to pull images if the cache operations fail for whatever reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot that steps within the same job don't share the cache. But I thought that's what the existing cache-to/from already solve.
| "--nsjail-args", | ||
| nargs="*", | ||
| default=[], | ||
| dest="nsjail_args", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the help output look like now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Old:
root@snekbox-6c47cc4bf5-9gchm:/snekbox# python -m snekbox --help
usage: snekbox [-h] code [nsjail_args ...] [--- py_args ...]
positional arguments:
code the Python code to evaluate
nsjail_args override configured NsJail options (default: [])
py_args arguments to pass to the Python process (default: ['-c'])
options:
-h, --help show this help message and exit
New:
root@a7cdb21501e0 /snekbox
❯ python -m snekbox --help
usage: snekbox [-h] code [nsjail_args ...] [--- py_args ...]
positional arguments:
code the Python code to evaluate
options:
-h, --help show this help message and exit
--nsjail-args [NSJAIL_ARGS ...]
override configured NsJail options (default: []) (default: [])
--py-args [PY_ARGS ...]
arguments to pass to the Python process (default: ['-c']) (default: ['-c'])
I can try find a solution that brings them closer together, they are functionally the same (hence no updates in the test cases) but the help is presented differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the default shown twice now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're now options rather than positional arguments. That's confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the default shown twice now?
We hard-coded it previously but now switching it from store_const appears to display it again, we can remove our hardcoding.
They're now options rather than positional arguments. That's confusing.
Yeah, I'll be honest I'd just updated it to maintain functionality because argparse in 3.14 broke it, hadn't looked at how it changed the help command.
I'll have a play around to see if I can maintain behaviour on 3.14 and keep the help command useful.
This PR makes a start at decoupling the specific setup of snekbox PyDis uses and snekbox as a standalone application.
This is done by building a basic snekbox application that supports evaluation using only the system version of Python.
We then have a new separate
Dockerfile.pydisthat uses the pre-built versions frompython-discord/python-builds.An integration specific image using this Dockerfile is built for integration tests to mirror the current behaviour.