From 8812d4a06b830e207ec7421d5e1767121f2cd8fa Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 30 Oct 2020 17:13:34 +0100 Subject: [PATCH 1/4] auth-helper: properly await replacement of the token value in the config After writing the `.extraheader` config, we manually replace the token with the actual value. This is done in an `async` function, but we were not `await`ing the result. In our tests, this commit fixes a flakiness we observed where `remote.origin.url` sometimes (very rarely, actually) is not set for submodules. Our interpretation is that the configs are in the process of being rewritten with the correct token value _while_ another `git config` that wants to set the `insteadOf` value is reading the config, which is currently empty. A more idiomatic way to fix this in Typescript would use `Promise.all()`, like this: await Promise.all( configPaths.map(async configPath => { core.debug(`Replacing token placeholder in '${configPath}'`) await this.replaceTokenPlaceholder(configPath) }) ) However, during review of https://github.com/actions/checkout/pull/379 it was decided to keep the `for` loop in the interest of simplicity. Reported by Ian Lynagh. Signed-off-by: Johannes Schindelin --- src/git-auth-helper.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/git-auth-helper.ts b/src/git-auth-helper.ts index fc1404c2f..291d83bc2 100644 --- a/src/git-auth-helper.ts +++ b/src/git-auth-helper.ts @@ -148,7 +148,7 @@ class GitAuthHelper { output.match(/(?<=(^|\n)file:)[^\t]+(?=\tremote\.origin\.url)/g) || [] for (const configPath of configPaths) { core.debug(`Replacing token placeholder in '${configPath}'`) - this.replaceTokenPlaceholder(configPath) + await this.replaceTokenPlaceholder(configPath) } if (this.settings.sshKey) { From db753157fd43ec7174150b80e9880bef01060d60 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 30 Oct 2020 17:39:47 +0100 Subject: [PATCH 2/4] downloadRepository(): await the result of recursive deletions Signed-off-by: Johannes Schindelin --- src/github-api-helper.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/github-api-helper.ts b/src/github-api-helper.ts index 8bbcf2dea..cadd13b6e 100644 --- a/src/github-api-helper.ts +++ b/src/github-api-helper.ts @@ -47,7 +47,7 @@ export async function downloadRepository( } else { await toolCache.extractTar(archivePath, extractPath) } - io.rmRF(archivePath) + await io.rmRF(archivePath) // Determine the path of the repository content. The archive contains // a top-level folder and the repository content is inside. @@ -70,7 +70,7 @@ export async function downloadRepository( await io.mv(sourcePath, targetPath) } } - io.rmRF(extractPath) + await io.rmRF(extractPath) } /** From e5da2adba07fae449a596fcdd473de5212346a87 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 30 Oct 2020 17:42:13 +0100 Subject: [PATCH 3/4] Ask ESLint to report floating Promises This rule is quite helpful in avoiding hard-to-debug missing `await`s. Note: there are two locations in `src/main.ts` that trigger warnings: the `run()` and the `cleanup()` function are called without `await` and without any `.catch()` clause. In the initial version of https://github.com/actions/checkout/pull/379, this was addressed by adding `.catch()` clauses. However, it was determined that this is boilerplate code that will need to be fixed in a broader way. Signed-off-by: Johannes Schindelin --- .eslintrc.json | 1 + 1 file changed, 1 insertion(+) diff --git a/.eslintrc.json b/.eslintrc.json index 02c79de1a..93bf9773a 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -27,6 +27,7 @@ "@typescript-eslint/no-empty-interface": "error", "@typescript-eslint/no-explicit-any": "error", "@typescript-eslint/no-extraneous-class": "error", + "@typescript-eslint/no-floating-promises": "error", "@typescript-eslint/no-for-in-array": "error", "@typescript-eslint/no-inferrable-types": "error", "@typescript-eslint/no-misused-new": "error", From 1607ea39c9111ae7b3b33e2e9a25ad5c4102fa6c Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 30 Oct 2020 17:43:31 +0100 Subject: [PATCH 4/4] Rebuild This trick was brought to you by `npm ci && npm run build`. Needed to get the PR build to pass. Signed-off-by: Johannes Schindelin --- dist/index.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dist/index.js b/dist/index.js index 9683c1140..e765cc2c0 100644 --- a/dist/index.js +++ b/dist/index.js @@ -5498,7 +5498,7 @@ class GitAuthHelper { const configPaths = output.match(/(?<=(^|\n)file:)[^\t]+(?=\tremote\.origin\.url)/g) || []; for (const configPath of configPaths) { core.debug(`Replacing token placeholder in '${configPath}'`); - this.replaceTokenPlaceholder(configPath); + yield this.replaceTokenPlaceholder(configPath); } if (this.settings.sshKey) { // Configure core.sshCommand @@ -9594,7 +9594,7 @@ function downloadRepository(authToken, owner, repo, ref, commit, repositoryPath) else { yield toolCache.extractTar(archivePath, extractPath); } - io.rmRF(archivePath); + yield io.rmRF(archivePath); // Determine the path of the repository content. The archive contains // a top-level folder and the repository content is inside. const archiveFileNames = yield fs.promises.readdir(extractPath); @@ -9613,7 +9613,7 @@ function downloadRepository(authToken, owner, repo, ref, commit, repositoryPath) yield io.mv(sourcePath, targetPath); } } - io.rmRF(extractPath); + yield io.rmRF(extractPath); }); } exports.downloadRepository = downloadRepository;