Allow multiple API tokens and app urls to be specified #499

edoardopirovano · 2024-08-23T13:19:09Z

This hasn't been fully tested yet, but putting it up for review so it can potentially be merged while I'm on holiday if needed.

vercel · 2024-08-23T13:19:13Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
report-diffs-action	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 4, 2024 4:09pm

alwaysmeticulous · 2024-08-23T13:19:58Z

✅ Meticulous spotted zero visual differences across 8 screens tested: view results.

_{Expected differences? Click here. Test suite: public-tests. Last updated for commit 829dde1. This comment will update as new commits are pushed.}

Que3216 · 2024-08-27T14:46:36Z

src/actions/cloud-compute/cloud-compute.ts

+  const { apiToken: apiTokenInput, appUrl: appUrlInput } =
+    getInCloudActionInputs();
+  const apiTokens = apiTokenInput.split(",").map((token) => token.trim());
+  const appUrls = appUrlInput.split(",").map((token) => token.trim());
+  while (appUrls.length < apiTokens.length) {
+    // If the number of app URLs is less than the number of API tokens, the
+    // last URL will be used for all remaining projects. This allow for
+    // more concise inputs when one app URL is to be used many times. In
+    // particular in the common case of one app URL for all projects.
+    appUrls.push(appUrls[appUrls.length - 1]);
  }


[nit] Since this is input parsing logic I'd push this down into getInCloudActionInputs to keep it separate from the main code

We should also update the docs here: https://github.com/alwaysmeticulous/report-diffs-action/blob/main/cloud-compute/action.yml.

That said wondering what the best API is for us here? We should work that out before shipping. Some notes here https://www.notion.so/API-for-multi-project-cloud-compute-action-75afd0edf8694bd7be54b305e4a63c30?pvs=4.

Thinking we should do this:

- name: Run Meticulous tests uses: alwaysmeticulous/report-diffs-action/cloud-compute@v1 with: # Important: remember to add the '|' character below: GitHub doesn't support complex action inputs, so we're actually passed the sub YAML as a string. projects_yaml: | admin_portal: api_token: ${{ secrets.METICULOUS_ADMIN_PORTAL_API_TOKEN }} app_url: https://localhost:3000/ skip: ${{ steps.changed-admin-portal-files.outputs.any_changed == 'false' }} customer_portal: api_token: ${{ secrets.METICULOUS_CUSTOMER_PORTAL_API_TOKEN }} app_url: https://localhost:3000/ skip: ${{ steps.changed-customer-portal-files.outputs.any_changed == 'false' }}

One thing I find useful on API design is always to try the API out / write out how user would use it. Much easier to iterate on correct API when looking at example usages across common use cases than by looking at the type definition of the API.

Que3216 · 2024-08-27T14:49:46Z

src/actions/cloud-compute/cloud-compute.ts

+    }
+  });
+  if (failureMessage) {
+    setFailed(failureMessage);


In the common case of just using one action we should probably keep the failure message as a clean as we can, similar to before e.g.

failures.push({ runNumber: index + 1, message })`

const message = failures.length === 1 ? failures[0].message : ...;

Que3216 · 2024-08-27T14:53:29Z

src/actions/cloud-compute/cloud-compute.ts

+    if (result.status === "rejected") {
+      const message =
+        result.reason instanceof Error ? result.reason.message : `${error}`;
+      failureMessage += `Run #${index + 1} failed: ${message}\n`;


Not sure how often this case would get hit (maybe quite rare?), but it could make sense to give some context.
Also maybe Run -> Test run so user doesn't think this is a retry number. And in the common case of just using one action we should probably keep the failure message as a clean as we can, similar to before e.g.

if (apiTokens.length > 1) { failureMessage += `Test run #${index + 1} against '${appUrls[index]}' with token '${apiTokens.slice(0, 3)}...': ${message}`; } else { failureMessage = message; }

Also, if we're not already logging it, maybe also:

logger.error(`[Test run #${index + 1}]`, result.reason);

Que3216 · 2024-08-27T14:54:27Z

src/actions/cloud-compute/cloud-compute.ts

  const { payload } = context;
  const event = getCodeChangeEvent(context.eventName, payload);
  const { owner, repo } = context.repo;
  const isDebugPRRun = isDebugPullRequestRun(event);
  const octokit = getOctokitOrFail(githubToken);
-  const logger = log.getLogger(METICULOUS_LOGGER_NAME);
+  const logger = getPrefixedLogger(`Run #${runNumber}`);


Ah nice!

[nit] Just in case user things this means retries

Suggested change

const logger = getPrefixedLogger(`Run #${runNumber}`);

const logger = getPrefixedLogger(`Test run #${runNumber}`);

Que3216 · 2024-08-27T14:55:12Z

src/actions/cloud-compute/cloud-compute.ts

+  ).forEach((result, index) => {
+    if (result.status === "rejected") {
+      const message =
+        result.reason instanceof Error ? result.reason.message : `${error}`;


Suggested change

result.reason instanceof Error ? result.reason.message : `${error}`;

result.reason instanceof Error ? result.reason.message : `${result.reason}`;

(error doesn't exist)

src/actions/cloud-compute/cloud-compute.ts

alexivanov · 2024-08-27T16:25:07Z

src/actions/cloud-compute/cloud-compute.ts

-          keepTunnelOpenPromise.resolve();
-        }, DEBUG_MODE_KEEP_TUNNEL_OPEN_DURAION.as("milliseconds"));
-      }
+        testSuiteId: "__meticulous_debug__",


we should probably tag this comment with the project ID too as this won't work in multi-project setups. Looks like this already doesn't work well on multi-project PRs.

github-actions · 2024-08-28T16:46:04Z

✅ Meticulous spotted zero visual differences across 8 screens tested: view results.

_{Test suite: test Meticulous with deployment url. Last updated for commit 829dde1. This comment will update as new commits are pushed.}

github-actions · 2024-08-28T16:46:31Z

✅ Meticulous spotted zero visual differences across 8 screens tested: view results.

_{Test suite: test Meticulous with app url. Last updated for commit 829dde1. This comment will update as new commits are pushed.}

Que3216

Nice!

edoardopirovano

Nice, the new syntax looks good to me! Just a few small nits on your commits @alexivanov 🙂

edoardopirovano · 2024-09-02T11:10:13Z

cloud-compute/action.yml

+        admin:
+          api-token: {{ secrets.METICULOUS_ADMIN_API_TOKEN }}
+          app-url: "http://localhost:4000"
+          skip: {{ secrets.METICULOUS_APP_API_TOKEN }}


I'm guessing this is a bad copy paste? Should probably be an example of using the output from a previous step to determine whether to run it or not?

edoardopirovano · 2024-09-02T11:12:09Z

src/actions/cloud-compute/cloud-compute.ts

+  // This is used to simplify some of the logging and error handling.
+  const isSingleTestRunExecution =
+    projectTargetsToRun.length === 1 &&
+    projectTargetsToRun[0].name === "default";


Hmm, I think we should use projectTargets (before the filtering is applied) here. If we have more than one project in the config, I think it's nice to be explicit in the logging about which one the logs are coming from even if a particular run only executed one.

edoardopirovano · 2024-09-02T11:13:04Z

src/actions/cloud-compute/run-test-run.ts

+  if (!project) {
+    throw new Error(
+      `Could not retrieve project data${
+        isSingleTestRunExecution ? "" : `for project ${testRunId}`


Suggested change

isSingleTestRunExecution ? "" : `for project ${testRunId}`

isSingleTestRunExecution ? "" : ` for project ${testRunId}`

edoardopirovano · 2024-09-02T11:14:00Z

src/actions/cloud-compute/consts.ts

@@ -0,0 +1,5 @@
+import { Duration } from "luxon";
+
+export const DEBUG_MODE_KEEP_TUNNEL_OPEN_DURAION = Duration.fromObject({


Suggested change

export const DEBUG_MODE_KEEP_TUNNEL_OPEN_DURAION = Duration.fromObject({

export const DEBUG_MODE_KEEP_TUNNEL_OPEN_DURATION = Duration.fromObject({

Further follow-up to #499

vercel bot temporarily deployed to preview August 23, 2024 13:19 Inactive

edoardopirovano force-pushed the edoardo/multiple-app-tokens branch from 3841435 to ca83958 Compare August 23, 2024 13:38

vercel bot temporarily deployed to preview August 23, 2024 13:38 Inactive

Allow multiple API tokens and app urls to be specified

f7f651c

edoardopirovano force-pushed the edoardo/multiple-app-tokens branch from ca83958 to f7f651c Compare August 23, 2024 15:13

vercel bot temporarily deployed to preview August 23, 2024 15:13 Inactive

edoardopirovano marked this pull request as ready for review August 23, 2024 15:20

edoardopirovano requested a review from a team August 23, 2024 15:20

Que3216 reviewed Aug 27, 2024

View reviewed changes

alexivanov reviewed Aug 27, 2024

View reviewed changes

alexivanov added 9 commits August 28, 2024 15:51

split in multiple files

90d9b14

move common params one level up

f703f36

Allow passing YAML for projects config in cloud compute action

2c29514

Use test run ID and fix some log messages

a70571d

key tunnel debug comment by test run ID

6a7de90

fix logger in older action

317239f

Add more logging

7925640

cleaner error message in single execution mode

be87cfd

build

490b876

vercel bot temporarily deployed to preview August 28, 2024 16:34 Inactive

alexivanov added 2 commits August 28, 2024 17:54

cleaner logging for single project case

87c74b1

pass correct argument

04a1672

vercel bot temporarily deployed to preview August 28, 2024 16:57 Inactive

alexivanov requested a review from Que3216 August 28, 2024 17:04

Que3216 approved these changes Aug 30, 2024

View reviewed changes

edoardopirovano commented Sep 2, 2024

View reviewed changes

vercel bot temporarily deployed to preview September 4, 2024 16:06 Inactive

Address my own comments

829dde1

edoardopirovano force-pushed the edoardo/multiple-app-tokens branch from 74744dc to 829dde1 Compare September 4, 2024 16:09

vercel bot temporarily deployed to preview September 4, 2024 16:09 Inactive

edoardopirovano merged commit f3281ad into main Sep 4, 2024
10 checks passed

edoardopirovano deleted the edoardo/multiple-app-tokens branch September 4, 2024 16:20

edoardopirovano added a commit that referenced this pull request Sep 4, 2024

Follow-ups to #499

bd49ecb

edoardopirovano added a commit that referenced this pull request Sep 4, 2024

Follow-ups to #499 (#504)

3537441

edoardopirovano mentioned this pull request Sep 4, 2024

Fix threading through of logger #505

Merged

edoardopirovano added a commit that referenced this pull request Sep 4, 2024

Fix threading through of logger (#505)

f263cb0

Further follow-up to #499

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow multiple API tokens and app urls to be specified #499

Allow multiple API tokens and app urls to be specified #499

edoardopirovano commented Aug 23, 2024 •

edited

Loading

vercel bot commented Aug 23, 2024 •

edited

Loading

alwaysmeticulous bot commented Aug 23, 2024 •

edited

Loading

Que3216 Aug 27, 2024

Que3216 Aug 27, 2024

Que3216 Aug 27, 2024

Que3216 Aug 27, 2024

Que3216 Aug 27, 2024

Que3216 Aug 27, 2024

Que3216 Aug 27, 2024

Que3216 Aug 27, 2024

alexivanov Aug 27, 2024

github-actions bot commented Aug 28, 2024 •

edited

Loading

github-actions bot commented Aug 28, 2024 •

edited

Loading

Que3216 left a comment

edoardopirovano left a comment

edoardopirovano Sep 2, 2024

edoardopirovano Sep 2, 2024

edoardopirovano Sep 2, 2024

edoardopirovano Sep 2, 2024

	const logger = getPrefixedLogger(`Run #${runNumber}`);
	const logger = getPrefixedLogger(`Test run #${runNumber}`);

	result.reason instanceof Error ? result.reason.message : `${error}`;
	result.reason instanceof Error ? result.reason.message : `${result.reason}`;

	isSingleTestRunExecution ? "" : `for project ${testRunId}`
	isSingleTestRunExecution ? "" : ` for project ${testRunId}`

		@@ -0,0 +1,5 @@
		import { Duration } from "luxon";

		export const DEBUG_MODE_KEEP_TUNNEL_OPEN_DURAION = Duration.fromObject({

Allow multiple API tokens and app urls to be specified #499

Allow multiple API tokens and app urls to be specified #499

Conversation

edoardopirovano commented Aug 23, 2024 • edited Loading

vercel bot commented Aug 23, 2024 • edited Loading

alwaysmeticulous bot commented Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 28, 2024 • edited Loading

github-actions bot commented Aug 28, 2024 • edited Loading

Que3216 left a comment

Choose a reason for hiding this comment

edoardopirovano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edoardopirovano commented Aug 23, 2024 •

edited

Loading

vercel bot commented Aug 23, 2024 •

edited

Loading

alwaysmeticulous bot commented Aug 23, 2024 •

edited

Loading

github-actions bot commented Aug 28, 2024 •

edited

Loading

github-actions bot commented Aug 28, 2024 •

edited

Loading