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

Parameterize autograder URL #1286

Merged

Conversation

Projects
None yet
2 participants
@c-w
Copy link
Contributor

commented Jun 8, 2018

Enabling the AUTOGRADER_URL value to be set by deployments of the application means that we don't have to rely on a single autograder deployment and other OK users such as Imperial College can deploy their own instances.

Note that it's a bit of a misnomer to have the AUTOGRADER_URL in the constants file but dereference it from an environment variable. Likely a more consistent place would be the Flask app settings. However, the autograder is implemented outside of the Flask app context so it might be a non-trivial change to pull the configuration from the Flask settings and it would unnecessarily couple the two. As such, reading the environment variable in the constants file seems like a pragmatic compromise.

Parameterize autograder URL
Enabling the `AUTOGRADER_URL` value to be set by deployments of the
application means that we don't have to rely on a single autograder
deployment and other OK users such as Imperial College can deploy their
own instances.

Note that it's a bit of a misnomer to have the `AUTOGRADER_URL` in the
constants file but dereference it from an environment variable. Likely a
more consistent place would be the Flask app settings. However, the
autograder is implemented outside of the Flask app context so it might
be a non-trivial change to pull the configuration from the Flask
settings and it would unnecessarily couple the two. As such, reading the
environment variable in the constants file seems like a pragmatic
compromise.

@c-w c-w force-pushed the icokpy:enhancement/c-w/parameterize-autograder-url branch from de9ddcd to adb22a4 Jun 8, 2018

@@ -33,7 +34,7 @@
TIMEZONE = 'America/Los_Angeles'
ISO_DATETIME_FMT = '%Y-%m-%d %H:%M:%S'

AUTOGRADER_URL = 'https://autograder.cs61a.org'
AUTOGRADER_URL = os.getenv('AUTOGRADER_URL', 'https://autograder.cs61a.org')

This comment has been minimized.

Copy link
@Sumukh

Sumukh Jun 8, 2018

Member

I'd prefer if we move this out of constants and into the config if we're going to read from the env vars. I don't mind merging this as is, but if it's not a lot of work - we should move it to the config & change the (two afaik) places we use in the code.

This comment has been minimized.

Copy link
@Sumukh

Sumukh Jun 8, 2018

Member

Note that it's a bit of a misnomer to have the AUTOGRADER_URL in the constants file but dereference it from an environment variable. Likely a more consistent place would be the Flask app settings. However, the autograder is implemented outside of the Flask app context so

Nevermind, I just read this.

@Sumukh

Sumukh approved these changes Jun 8, 2018

@Sumukh Sumukh merged commit b0eb2be into okpy:master Jun 8, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
ci/circleci Your tests passed on CircleCI!
Details

@Sumukh Sumukh deleted the icokpy:enhancement/c-w/parameterize-autograder-url branch Jun 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.