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

feat: continue on node package install failure #236

Conversation

puzzler7
Copy link
Contributor

@puzzler7 puzzler7 commented Apr 3, 2024

Problem

Currently, most of our check-on-prs/check nightly failures occur while installing node packages, for a variety of reasons. Right now, this causes the check to fail, and we report that "something went wrong". Instead, we may want to disable linters that depend on node-modules (specifically, stylelint and eslint) and lint the remaining files to report our best-effort results.

Pros:

  • We would report some issues, and some issues is better than no issues
  • We wouldn't be erroring with a message that seems like an internal error (because for all we know right now, it could be)

Cons:

  • A user could introduce a lint issue in the same PR as a breaking change to package.json and we would pass that PR
  • I am pretty sure that we don't have a great way of surfacing to the user that we're disabling eslint without writing an annoying amount of boilerplate to send the data through the cli through services back to github
    • We do log this in the action logs, but that's the most visible place

Solution

After talking with Sam about this, we figured the best solution is to disable the linters if we are auto-initing for the user, but not if the user has a trunk.yaml and therefore has deliberately opted to see ts/js issues.

Testing

Added a repo test for prawn-test-staging-rw/node-packages-failure-test that has an invalid package.json, and checks to make sure the action disables eslint and stylelint, and continues after the failure.

Comment on lines 140 to 141
${TRUNK_PATH} check disable eslint
${TRUNK_PATH} check disable stylelint
Copy link
Contributor Author

@puzzler7 puzzler7 Apr 3, 2024

Choose a reason for hiding this comment

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

Checked with Tyler that these are all of the linters that depend on node packages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can apply them in one call, i.e. ${TRUNK_PATH} check disable eslint stylelint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Combined those, as well as the couple other places we disable multiple linters.

@puzzler7 puzzler7 merged commit da67635 into main Apr 4, 2024
38 of 40 checks passed
@puzzler7 puzzler7 deleted the maverick/trunk-9905-continue-in-check-on-prscheck-nightly-if-node-install-fails branch April 4, 2024 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants