Properly extract commit sha from PullRequests #186

nulltoken · 2020-05-01T12:16:24Z

Fix an issue discovered by @philsturgeon in https://github.com/protect-earth/protect.earth/pull/61/checks?check_run_id=634958740

Also:

slightly enhance the logging
run the analysis before attempting to create a checkrun (this will fail when PR is created from a fork as the token isn't granted with enough rights to create annotations), in order to provide minimal feedback.
re-use Spectral stylish formater to produce a cleaner textual report
do not fail the action upon insufficient rights (would happen when a PR originates from a fork) => should only fail the build upon fatal linting errors and unexpected errors

nulltoken · 2020-05-01T12:19:13Z

@philsturgeon @XVincentX
For PR originating from forks, one enhancement would be to re-use the stylish cli formatter from spectral in order to provide actionable feedback (even in textual form).
Another idea would be to check if using problem matchers (#184) might actually solve the permission issue as well.

nulltoken · 2020-05-01T12:20:26Z

One can check the proposed behavior of that branch in protect-earth/protect.earth#62

This gives a chance for feedback when pull requests are opened from a fork. In that case, the token is not granted with enough rights to create annotations in the upstream repository.

nulltoken · 2020-05-01T14:27:37Z

@philsturgeon @XVincentX
For PR originating from forks, one enhancement would be to re-use the stylish cli formatter from spectral in order to provide actionable feedback (even in textual form).

nulltoken · 2020-05-01T15:36:18Z

src/octokit.ts

@@ -16,18 +23,33 @@ type Event = {

 export const createOctokitInstance = (token: string) => TE.fromEither(E.tryCatch(() => new GitHub(token), E.toError));

+const tryCreateGithubCheck = async (


@XVincentX Could you please help me rewrite this in a more fp-ts way? I haven't found the proper way to do this.

You really can't, since the underlying API has side-effects. You can wrap it (as you're doing) with tryCatch later

XVincentX

I took a look and unfortunately I think it's going in the wrong direction regards the usage of fp-ts. Also, bundling so many changes together didn't really help the review.

I would suggest to close this, open a new PR fixing exclusively the bug. In meantime I can try to look at this during the weekend and cleanup some of the stuff.

To give you some context, this was born as a personal pet project to experiment functional concepts that "escalated" to be then the official action linter.

XVincentX · 2020-05-01T20:59:44Z

src/index.ts

-          })
-        )
-      );
+      const spectralRuns = parsed.map(v => pipe(runSpectral(spectral, v)));


If you're running only a single operation, you do not need to pipe

Suggested change

const spectralRuns = parsed.map(v => pipe(runSpectral(spectral, v)));

const spectralRuns = parsed.map(v => runSpectral(spectral, v));

XVincentX · 2020-05-01T21:03:42Z

src/index.ts

+        pipe(
+          getRepositoryInfoFromEvent(GITHUB_EVENT_PATH, INPUT_EVENT_NAME),
+          TE.map(event => ({ event }))
+        ),
+        TE.chain(({ event }) =>
+          pipe(
+            readFilesToAnalyze(INPUT_FILE_GLOB, GITHUB_WORKSPACE),
+            TE.chain(fileContents => createSpectralAnnotations(INPUT_SPECTRAL_RULESET, fileContents, GITHUB_WORKSPACE)),
+            TE.map(annotations => ({ event, annotations }))
+          )
+        ),
+        TE.chain(({ event, annotations }) =>


Hmm I'd say this is going in the wrong direction. I'll probably have to check this out.

XVincentX · 2020-05-01T21:04:39Z

src/octokit.ts

@@ -16,18 +23,33 @@ type Event = {

 export const createOctokitInstance = (token: string) => TE.fromEither(E.tryCatch(() => new GitHub(token), E.toError));

+const tryCreateGithubCheck = async (


You really can't, since the underlying API has side-effects. You can wrap it (as you're doing) with tryCatch later

XVincentX · 2020-05-01T21:05:13Z

src/octokit.ts

+      return event.after;
+    default:
+      warning(event);
+      throw new Error(`Unsupported event '${eventName}'`);


Well, the idea of fp-ts is not to throw exception, so this is not ok. This should return an Either and a Left instance when something goes wrong.

nulltoken · 2020-05-02T08:11:12Z

I would suggest to close this, open a new PR fixing exclusively the bug.

Done with #187

In meantime I can try to look at this during the weekend and cleanup some of the stuff.

Ok. I'll learn by reading your code, then.

From a usage standpoint, the two most important features that this PR tried to add were:

Do not fail the action when the Github token doesn't bear enough rights to create annotations (which happens when the PR to be linted originates from a fork)
Provide the user with some textual report (Also helpful when a PR comes from a fork)

Bonus features (for potential later troubleshooting):

Log the event to which this action responds to
Log the commit sha being processed

nulltoken requested a review from philsturgeon May 1, 2020 12:16

nulltoken self-assigned this May 1, 2020

nulltoken requested a review from XVincentX May 1, 2020 12:17

nulltoken added 5 commits May 1, 2020 16:03

chore: fail the action upon error

9898643

chore: enhance logging on error

995ee8d

chore: log event that triggered the action

cb92023

fix: use dedicated sha extractions per kind of event

bcc3ea9

chore: run analysis before creating the check run

7c10d15

This gives a chance for feedback when pull requests are opened from a fork. In that case, the token is not granted with enough rights to create annotations in the upstream repository.

nulltoken force-pushed the ntk/logging branch from 9f380b0 to 0d376df Compare May 1, 2020 14:03

chore: produce a textual output of the linting

0575095

nulltoken force-pushed the ntk/logging branch from 0d376df to 0575095 Compare May 1, 2020 14:11

chore: do not fail the build on insufficient permissions

a9669f3

nulltoken commented May 1, 2020

View reviewed changes

XVincentX suggested changes May 1, 2020

View reviewed changes

nulltoken closed this May 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly extract commit sha from PullRequests #186

Properly extract commit sha from PullRequests #186

nulltoken commented May 1, 2020 •

edited

Loading

nulltoken commented May 1, 2020

nulltoken commented May 1, 2020

nulltoken commented May 1, 2020

nulltoken May 1, 2020

XVincentX May 1, 2020

XVincentX left a comment

XVincentX May 1, 2020

XVincentX May 1, 2020

XVincentX May 1, 2020

XVincentX May 1, 2020

nulltoken commented May 2, 2020

		@@ -16,18 +23,33 @@ type Event = {

		export const createOctokitInstance = (token: string) => TE.fromEither(E.tryCatch(() => new GitHub(token), E.toError));

		const tryCreateGithubCheck = async (

	const spectralRuns = parsed.map(v => pipe(runSpectral(spectral, v)));
	const spectralRuns = parsed.map(v => runSpectral(spectral, v));

Properly extract commit sha from PullRequests #186

Properly extract commit sha from PullRequests #186

Conversation

nulltoken commented May 1, 2020 • edited Loading

nulltoken commented May 1, 2020

nulltoken commented May 1, 2020

nulltoken commented May 1, 2020

nulltoken May 1, 2020

Choose a reason for hiding this comment

XVincentX May 1, 2020

Choose a reason for hiding this comment

XVincentX left a comment

Choose a reason for hiding this comment

XVincentX May 1, 2020

Choose a reason for hiding this comment

XVincentX May 1, 2020

Choose a reason for hiding this comment

XVincentX May 1, 2020

Choose a reason for hiding this comment

XVincentX May 1, 2020

Choose a reason for hiding this comment

nulltoken commented May 2, 2020

nulltoken commented May 1, 2020 •

edited

Loading