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

Admin api routes #166

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open

Admin api routes #166

wants to merge 27 commits into from

Conversation

ananthm0203
Copy link
Contributor

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.

@ananthm0203 ananthm0203 requested a review from nd-0r February 18, 2024 21:40
.gitignore Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
@nd-0r
Copy link
Member

nd-0r commented Feb 20, 2024

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 git-rm'ing all the __pycache__ files and addressing the comments about the gitignore and the requirements.txt? Thanks!

src/auth.py Show resolved Hide resolved
@nd-0r
Copy link
Member

nd-0r commented Feb 21, 2024

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:

  1. Add the assignment
  2. Add the extensions in one batched API request
  3. Add the scheduled runs for the full roster and extensions in one batched API request

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?

@ananthm0203
Copy link
Contributor Author

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:

  1. Add the assignment
  2. Add the extensions in one batched API request
  3. Add the scheduled runs for the full roster and extensions in one batched API request

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.

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.

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 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.

@nd-0r
Copy link
Member

nd-0r commented Feb 21, 2024

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 .roster repo and have that automatically be sent to broadway via a webhook. But I agree that this has already been a bit of a time sink. We can talk about this at the infra meeting.

Copy link
Member

@nd-0r nd-0r left a 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!

src/routes_api.py Outdated Show resolved Hide resolved
return util.error("Invalid course or assignment.\nPlease try again.")


missing = util.check_missing_fields(form, "netids", "max_runs", "start", "end")
Copy link
Member

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.

Copy link
Contributor Author

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?

missing = util.check_missing_fields(form, "run_time", "due_time", "name", "config")
What input did you put in to throw this error?

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:
Copy link
Member

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?

Copy link
Contributor Author

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants