Improve error handling and further improve the logic #7

devlato · 2021-11-18T04:53:51Z

Further improve error handling and logic

Improving error handling and further logic

devlato · 2021-11-18T04:59:18Z

Hey @kt3k – sorry for annoyance, here's another follow-up one 🙂 Probably the last one

devlato · 2021-11-18T05:00:08Z

index.js

+  const url = `/repos/${credentials.owner}/${credentials.repo}/pulls/${pullNumber}`;
+
+  core.info(`Making PATCH request to "${url}" with params ${JSON.stringify(params)}`);
+  await octokit.request(`PATCH ${url}`, params);


Making the request manually, because of a strange error 'Body must be a JSON' that the octokit.pulls.update returns

https://docs.github.com/en/rest/reference/pulls#update-a-pull-request--code-samples

I tested it on my other repo and it worked like a charm (and both PR title and PR body were updated as expected), while the pulls.update threw the above error

devlato · 2021-11-18T05:00:23Z

index.js

  })
  .catch((e) => {
-    core.setFailed(e.message);
+    core.setFailed(e.stack || e.message);


Stack trace's better than only a message, right?

devlato · 2021-11-18T05:09:09Z

index.js

+
+  const url = `/repos/${credentials.owner}/${credentials.repo}/pulls/${pullNumber}`;
+
+  core.info(`Making PATCH request to "${url}" with params ${JSON.stringify(params)}`);


This is safe, because:

GitHub masks all secrets in the logs

In addition, params doesn't contain any sensitive information anyway

devlato · 2021-11-18T05:10:12Z

index.js

-    core.info(`No such pull request: source=${source} base=${base}`);
-    return;
+  if (pulls.length === 0) {
+    throw new Error(`No pull request found for source="${source || '<not found>'}" base=${baseBranch || '<not specified>'}`);


throw would stop the script execution here and would be reported to the core

devlato · 2021-11-18T05:10:18Z

index.js

-  core.info(`Found pull request #${number}`);
+  const pullRequest = pulls.find((p) => p.state === 'open');
+  if (pullRequest == null) {
+    throw new Error(`No open pull request found for source="${source || '<not found>'}" base=${baseBranch || '<not specified>'}`);


devlato · 2021-11-18T05:10:39Z

index.js


-  const { number } = pulls[0];
-  core.info(`Found pull request #${number}`);
+  const pullRequest = pulls.find((p) => p.state === 'open');


This is (technically) safer than pulls[0]

devlato · 2021-11-18T05:22:11Z

@kt3k please take a look when you can, and if it looks fine, please merge and release to the marketplace 🙂

(cherry picked from commit 081bc30)

devlato · 2021-11-18T06:18:30Z

deps.js

@@ -0,0 +1 @@
+console.info(require('child_process').execSync('npm ci --ignore-scripts', { cwd: __dirname }).toString());


This handles npm packages pre-installation (in a fast CI-compliant way), instead of storing node_modules/ in it. When node_modules/ contain binaries, they won't run anyway

Is this recommended way to remove node_modules from repository?

The doc seems recommending the use of ncc

When node_modules/ contain binaries, they won't run anyway

I don't think our dependency includes any binary

I don't think our dependency includes any binary

Yeah no binaries in our deps atm

The doc seems recommending the use of ncc

I'd say safer to use a docker images for those scenarios, as ncc still builds a platform and architecture-specific binary

Is this recommended way to remove node_modules from repository?

Nope, technically, there's no recommended way – this one's straightforward though, and still guarantees the result.
The docs kinda suggest just ignoring it and keeping node_modules/ in the repo but I can name, like, a hundred reasons why it's totally wrong – I'm 100% sure this is just a bloody hack GitHub devs had to suggest to get away with

kt3k · 2021-11-18T07:09:23Z

index.js

-    core.info(`No such pull request: source=${source} base=${base}`);
-    return;
+  if (pulls.length === 0) {
+    throw new Error(`No pull request found for a source branch "${sourceBranch || '<not found>'}" and a base branch "${baseBranch || '<not specified>'}"`);


Does this mean the action fails in this case? In our use case, this needs to be success (When no need of update, we want this action to be just silent.

@kt3k right, but what's the scenario where the workflow gets triggered but no pull request exists? I assume this would be a developer mistake (e.g. they've subscribed to a wrong event) – wouldn't it be better to fail and let them know that something's wrong and let them fix it, rather than just keep running and silently consuming their GitHub actions monthly minutes allowance?

Ok. Thought twice, and I think your suggestion is correct. Let's try it

kt3k

LGTM

devlato and others added 2 commits November 18, 2021 15:53

Update index.js

01989e0

Improving error handling and further logic

Update package locks

79e8a99

devlato commented Nov 18, 2021

View reviewed changes

devlato changed the title ~~Improve error handling and further logic~~ Improve error handling and further improve the logic Nov 18, 2021

devlato commented Nov 18, 2021

View reviewed changes

Minor updates

fa47a8b

Clean up node_modules/ in favour of a pre-install step

7895d05

(cherry picked from commit 081bc30)

devlato commented Nov 18, 2021

View reviewed changes

kt3k reviewed Nov 18, 2021

View reviewed changes

kt3k approved these changes Nov 18, 2021

View reviewed changes

kt3k merged commit c23dd69 into kt3k:master Nov 18, 2021


		const url = `/repos/${credentials.owner}/${credentials.repo}/pulls/${pullNumber}`;

		core.info(`Making PATCH request to "${url}" with params ${JSON.stringify(params)}`);

		@@ -0,0 +1 @@
		console.info(require('child_process').execSync('npm ci --ignore-scripts', { cwd: __dirname }).toString());

Improve error handling and further improve the logic #7

Improve error handling and further improve the logic #7

Uh oh!

Conversation

devlato commented Nov 18, 2021

Uh oh!

devlato commented Nov 18, 2021

Uh oh!

devlato Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devlato Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devlato commented Nov 18, 2021

Uh oh!

devlato Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devlato Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devlato Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kt3k left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

devlato Nov 18, 2021 •

edited

Loading

devlato Nov 18, 2021 •

edited

Loading

devlato Nov 18, 2021 •

edited

Loading

devlato Nov 18, 2021 •

edited

Loading

devlato Nov 18, 2021 •

edited

Loading