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

Better config reload error handling #1522

Merged
merged 5 commits into from
May 18, 2022
Merged

Conversation

damianhxy
Copy link
Member

@damianhxy damianhxy commented May 14, 2022

Modify course and assessment config reload code to better handle cases where an invalid config is provided

Description

  • Modified course reload.html to be consistent with assignment reload.html
  • Validate syntax of config (via compilation) before saving
  • Correctly catch (certain classes of) exceptions when attempting to load config files
  • Make backups of last-known good configs when reloading

Motivation and Context

Currently, course config reload does not set the @error variable, so the error page is not correctly rendered.

Furthermore, config reload does not check the syntax of the config file before saving it, meaning that it's possible to reload a file with invalid syntax and corrupt the config.

Also fixes #1516

How Has This Been Tested?

  • Under normal circumstances, course config reload still works
  • Under normal circumstances, assessment config reload still works
  • When reloading, observe that a backup copy of the original config is made
  • Corrupt course config courses/<course-name>/course.rb and reload config. Error should be correctly caught and config in courseConfig/<course-name>.rb unaffected
  • Corrupt assessment config courses/<course-name>/<assessment-name>/<assessment-name>.rb and reload config. Error should be correctly caught and config in assessmentConfig/<course-name>-<assessment-name>.rb unaffected

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have run rubocop for style check. If you haven't, run overcommit --install && overcommit --sign to use pre-commit hook for linting
  • My change requires a change to the documentation, which is located at Autolab Docs
  • I have updated the documentation accordingly, included in this PR

Copy link
Contributor

@michellexliu michellexliu left a comment

Choose a reason for hiding this comment

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

see comments

@damianhxy damianhxy requested a review from michellexliu May 17, 2022 01:48
@damianhxy
Copy link
Member Author

Note that certain classes of errors (such as NameError and ArgumentError) will still not be caught. If a user introduces such an error they'll be stuck. There should be a way to gracefully recover from such a mistake (or at the very least, make a backup of last-known good config)

Copy link
Contributor

@michellexliu michellexliu left a comment

Choose a reason for hiding this comment

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

lgtm!

@damianhxy damianhxy merged commit c7567d0 into master May 18, 2022
@damianhxy damianhxy deleted the reload-config-error-handling branch May 18, 2022 00:31
victorhuangwq added a commit that referenced this pull request May 27, 2022
This was referenced May 28, 2022
damianhxy added a commit that referenced this pull request May 28, 2022
* Check for config existence

* Delete config copy

* Update edit course buttons

* Check for course config existence

* Delete course config file copies

* Reload course config directly
tuhe added a commit to tuhe/Autolab that referenced this pull request Jun 1, 2022
…ndling

Revert "Better config reload error handling (autolab#1522)"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

course reload diagnostics always fail
2 participants