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.

Sign up for GitHub

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

Jump to bottom

Help Gerald handle files that it can't read. #80

Merged
merged 11 commits into from
Aug 9, 2023
Merged

Help Gerald handle files that it can't read. #80

merged 11 commits into from
Aug 9, 2023

Conversation

jeresig
Copy link
Member

@jeresig jeresig commented Aug 8, 2023 •
edited
Loading

Summary:

This addresses a few Gerald issues:

  • If it can't read a file (because it's a symlink, or because it's a submodule, or something else) then it just returns an empty string, which is fine for our purposes.
  • If there is an error during execution we output the error and don't cause the Gerald Workflow to fail (this has caused a lot of confusion for engineers). It'll make our job in FEI harder to find failing jobs, but the alternative is unfortunately worse. Ideally we'd hook in to Slack or something to notify about failures.
  • It prints out the full diff of all the files - unfortunately for large diffs this causes some failures so I've turned that off.

Issue: FEI-4624

Test plan:

The test suite passed.

@jeresig jeresig self-assigned this Aug 8, 2023
@jeresig jeresig requested a review from a team August 8, 2023 19:50
@khan-actions-bot khan-actions-bot requested a review from a team August 8, 2023 19:50
@khan-actions-bot
Copy link
Contributor

khan-actions-bot commented Aug 8, 2023 •
edited
Loading

Gerald

Required Reviewers
  • @Khan/github-actions for changes to yarn.lock, dist/index.js, dist/source-map-support.js, src/execCmd.js, src/fs.js, src/gerald.js, src/setup.js, src/utils.js, .github/workflow-templates/_setup.yml, .github/workflow-templates/pr-actions.yml, .github/workflows/build.yml, .github/workflows/pr-actions.yml, .github/workflows/pr-autofix.yml, src/__test__/main.test.js, src/__test__/utils.test.js

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

kevinbarabash
kevinbarabash approved these changes Aug 8, 2023
View reviewed changes
Copy link

@kevinbarabash kevinbarabash left a comment

Choose a reason for hiding this comment

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

Code changes look good to me. Thanks for fixing this issue. 💚

src/gerald.js
Comment on lines +25 to +28
// NOTE(john): We could use core.setFailed here, but it means that
// Gerald fails sometimes in ways that are confusing to the user.
// Instead we print out the error for further debugging.
console.error(error);
Copy link

Choose a reason for hiding this comment

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

👍

@jeresig jeresig merged commit b3b278d into main Aug 9, 2023
7 checks passed
@jeresig jeresig deleted the FEI-4624 branch August 9, 2023 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinbarabash kevinbarabash kevinbarabash approved these changes

Assignees

@jeresig jeresig

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jeresig @khan-actions-bot @kevinbarabash