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

DM-46095: Improve GitHub check run feedback #80

Merged
merged 5 commits into from
Sep 5, 2024
Merged

Conversation

jonathansick
Copy link
Member

@jonathansick jonathansick commented Sep 4, 2024

  • Refactor functionality into the GitHubCheckRunService from the GitHubRepoService
  • Improve check run feedback when YAML cannot be parsed
  • Improve check run feedback when Jinja templating fails
  • Ensure that check runs always resolve and don't hang indefinitely. Defer to a subsequent PR that will also work with Noteburst for better feedback on notebook executions that time out.

Example of catching the YAMLError when parsing invalid YAML:

CleanShot 2024-09-04 at 12 54 11@2x

Example of a Jinja formatting error:

CleanShot 2024-09-04 at 15 06 11@2x

Now the methods related to servicing GitHub Check Runs are part of the
GitHubCheckRunService, which is apart from the GitHubRepoService. This
should help make it easier to focus on the check run flow, apart from
the functionality needed to sync data from GitHub.
If the times-square.yaml file or the sidecar YAML files can't be parsed,
we now format a check run annotation specifically to give feedback about
that.
@jonathansick jonathansick force-pushed the tickets/DM-46095 branch 2 times, most recently from 814e0a1 to 3d20876 Compare September 4, 2024 18:58
@jonathansick jonathansick changed the title Refactor into new GitHubCheckRunService DM-46095: Improve GitHub check run feedback Sep 4, 2024
If the Jinja templating in a notebook is incorrect, the
PageModel.render_parameters method raises a new exception,
PageJinjaError. This is captured in the notebook execution check run and
turned into useful user feedback.
@jonathansick jonathansick marked this pull request as ready for review September 4, 2024 20:35
@jonathansick jonathansick merged commit 4f77663 into main Sep 5, 2024
4 checks passed
@jonathansick jonathansick deleted the tickets/DM-46095 branch September 5, 2024 18:53
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.

1 participant