-
Notifications
You must be signed in to change notification settings - Fork 4
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
Admin api routes #166
base: master
Are you sure you want to change the base?
Admin api routes #166
Conversation
… into admin-api-routes
…-on-demand into admin-api-routes
Hey @ananthm0203, looking at these changes along with the CLI now. Did you get a chance to test the CLI with On-Demand locally? If not, just let me know and I can take care of that. In any case, would you mind |
I'm starting to realize some issues. Specifically, what happens when a batched request doesn't succeed? For reference, here's my understanding of the CLI's workflow so far:
For example, what if the API fails to commit a scheduled run for an extension midway through the CLI's deployment? Should the CLI roll-back the changes by deleting all changes for the assignment? Should the CLI just skip the scheduled run? What if the user wants to retry the same deployment again? I think, ideally, any progress should roll-back if anything in the deployment fails because the CLI should be completely stateless. One way I can think of to implement this would be to move the CLI functionality into one super-route in the API. Another way is to keep the CLI design as-is, but add routes to delete changes in the API and add some cleanup functions in the CLI. This would probably require returning identifiers for the database objects to the CLI. Along this same line of thinking, we could separate the process into adding the assignment+(roster scheduled run) and adding the extensions+(extension scheduled runs); then, we'd only roll back if the assignment process fails and print out the netids of the extensions fail. Thoughts? |
I do think the API should support delete functionalities, and while I considered a "super-route" in the API (which would keep stuff stateless), that doesn't seem like very good design, ideally you'd want an API to be modular.
I think this is a far more extensible approach, and something I considered, but by then I had already bled a lot of time here, so I wanted to push what I had and add updates to this in the future. The advantage of this approach is you could then use the CLI directly to add "default extensions" (which don't require too much configuration), and that could allow for other people to add extensions directly without knowing the structure of the JSON. |
Yeah, either way would require some significant redesign. I'm realizing I really just want the course admin to be able to add extensions in the |
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.
Looks good. Just those three things in the API routes. Let me know you don't have time to address them and I'd be happy to do it this weekend. Thanks again for working on this!
return util.error("Invalid course or assignment.\nPlease try again.") | ||
|
||
|
||
missing = util.check_missing_fields(form, "netids", "max_runs", "start", "end") |
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.
If the user neglects to add a config when adding extensions with a grading run, the app will throw an exception and have to be restarted (which happens automatically, but still). We should probably check to make sure the config is present before calling add_or_edit_scheduled_run
. Obviously, please correct me if I'm wrong on this.
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.
add_or_edit_scheduled_run
should check for that already, no?
broadway-on-demand/src/routes_api.py
Line 187 in e8a9086
missing = util.check_missing_fields(form, "run_time", "due_time", "name", "config") |
run_id = db.generate_new_id() | ||
retval = add_or_edit_scheduled_run(cid, aid, run_id, run_config, None) | ||
# TODO: There should be a better distinction between good and bad responses | ||
if retval[1] != HTTPStatus.OK: |
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.
If the run fails, can we just copy the code from staff_delete_scheduled_run
here to roll back the previously-scheduled runs?
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 might need some more clarification on what you mean here, are you saying to delete the run from sched_api
as a precaution, or are you saying that if any run fails to be added, we should remove all scheduled runs from the assignment? I'm not sure I agree with the second approach as much, in all honesty, but I guess I can see why we would want to do that.
Removed duplicate form=request.json
Extended API routes to be able to add/edit assignments, add extensions, and add scheduled grader runs. These require admin permissions and course token authentication. This is originally for the broadway-cli on-demand integration.