Skip to content

Commit

Permalink
Add missing awaits (#379)
Browse files Browse the repository at this point in the history
* 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 #379
it was decided to keep the `for` loop in the interest of simplicity.

Reported by Ian Lynagh.

Signed-off-by: Johannes Schindelin <[email protected]>

* downloadRepository(): await the result of recursive deletions

Signed-off-by: Johannes Schindelin <[email protected]>

* 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 #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 <[email protected]>

* 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 <[email protected]>
  • Loading branch information
dscho authored Nov 3, 2020
1 parent c952173 commit 5a4ac90
Show file tree
Hide file tree
Showing 4 changed files with 7 additions and 6 deletions.
1 change: 1 addition & 0 deletions .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
6 changes: 3 additions & 3 deletions dist/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand All @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/git-auth-helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions src/github-api-helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -70,7 +70,7 @@ export async function downloadRepository(
await io.mv(sourcePath, targetPath)
}
}
io.rmRF(extractPath)
await io.rmRF(extractPath)
}

/**
Expand Down

0 comments on commit 5a4ac90

Please sign in to comment.