Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

[WIP] Core Sandbox Logic #1

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

[WIP] Core Sandbox Logic #1

wants to merge 6 commits into from

Conversation

hariamoor-zz
Copy link
Collaborator

This follows the "Sandbox Generation Module" ticket - if anyone seeing this needs access, reach out to @olagun.

@hariamoor-zz hariamoor-zz added enhancement New feature or request help wanted Extra attention is needed labels May 29, 2020
@hariamoor-zz hariamoor-zz self-assigned this May 29, 2020
@hariamoor-zz
Copy link
Collaborator Author

The Dockerfile for the given Golang test works locally for me (through the terminal) using Docker v19.03 (community edition) and Go v1.14.

I'm currently getting a HTTP 500 error from my Docker server, so I can't run my unittest locally.

@alimohamad We talked about setting up K8S before moving forward with this. If you can finish that in the next 2-3 weeks, that'd be ideal. If there turns out to be something that blocks us on that, then we can give local testing another shot.

@hariamoor-zz hariamoor-zz marked this pull request as draft May 29, 2020 07:06
sandboxes/sandbox.py Show resolved Hide resolved
sandboxes/sandbox.py Outdated Show resolved Hide resolved
sandboxes/sandbox.py Outdated Show resolved Hide resolved
sandboxes/sandbox.py Outdated Show resolved Hide resolved
sandboxes/docker_files/tests/test_sandbox.py Show resolved Hide resolved
sandboxes/sandbox.py Outdated Show resolved Hide resolved
Copy link

@hemangandhi hemangandhi left a comment

Choose a reason for hiding this comment

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

Left some stuff about the parts I'm sure I would alter somehow (and Sri didn't already mention)

But I also have a general confusion about how the directories are laid out.

  1. Why is there an init.py in the root directory?
  2. Why is there an init.py under the sandboxes/docker_files directory?
  3. What does the tests directory signify? It looks like testing data, not the actual code that runs the tests. But it seems to contain both.
  4. The differing extensions for the dockerfiles seems to be confusing at least the Github syntax highlighting and I don't think it makes sense.

I think it'd make more sense if the python code used to run the sandboxes and the dockerfiles were separated from the project root. Perhaps like:

  • projects: the root for all the project templates
    • test_go: the go code and Dockerfile
    • test_python: the python code (though I don't think this is the code you have -- probably another main function) and dockerfile
  • lib (or something -- whatever pip convention dictates): to contain the python library
    • sandbox.py
  • test: the tests for the sandbox library

I guess I have https://realpython.com/python-application-layouts/#installable-single-package in mind with an extra dir for the project roots. I think that dockerfiles can reference each other, so duplication shouldn't be an issue if the projects get larger.

Pipfile Show resolved Hide resolved
FROM python:latest
COPY $PROJECT_ROOT .
RUN pip install .
CMD ["python", "-m", "unittest"]

Choose a reason for hiding this comment

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

I'm not sure about why this is just running tests vs. running an app like above?

Copy link
Collaborator Author

@hariamoor-zz hariamoor-zz May 30, 2020

Choose a reason for hiding this comment

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

The Dockerfiles aren't stable yet - that's why this is still WIP. They'll become much more fancy down the line once we get venue for testing. This will be resolved at merge-time.

Edit: Leaving this unresolved so I remember.

sandboxes/sandbox.py Outdated Show resolved Hide resolved
sandboxes/sandbox.py Show resolved Hide resolved
@hariamoor-zz
Copy link
Collaborator Author

@Sail338

All your comments have been applied - see the commits I linked.

@hemangandhi

See #2 in reference to your directory structure suggestion. We'll keep this PR short, because it's pretty blown up as it is (since we're waiting for @alimohamad on k8s).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants