Add optional commit SHA in config. #93
Conversation
jefchien
force-pushed
the
add-optional-commit-sha
branch
from
4ab55a4
to
35c0db2
Compare
jefchien
force-pushed
the
add-optional-commit-sha
branch
from
35c0db2
to
dd836ac
Compare
| @@ -235,7 +235,11 @@ async function getCommitFromGitHubAPIRequest(githubToken: string): Promise<Commi | |||
| }; | |||
| } | |||
|
|
|||
| async function getCommit(githubToken?: string): Promise<Commit> { | |||
| async function getCommit(githubToken?: string, commitSha?: string): Promise<Commit> { | |||
| if (commitSha && githubToken) { | |||
There was a problem hiding this comment.
Should this line be placed after the line 253-261 ? Mostly I can't see any reason to let this success faster though.
There was a problem hiding this comment.
Well, if you've provided the commit SHA, then your intention is for the action to use it. I don't see why it should even attempt to use the other options.
|
Hi @jefchien Thank you for your contribution! Could you please explain to me a bit more why would we want to provide the SHA as an input rather than just using the one provided via pull request or fall back to the current commit's sha? |
|
Hi @ktrz, So, in my use case, the workflow where the action is used can be triggered by a workflow dispatch where the SHA is provided as an input. This allows us to run the benchmarking after successful CI workflow runs or on selected commits. The issue I've found is that the |
|
Is running the That way you can make the branch point to the commit you want, and run the workflow from that branch, so that the commit is correct and you don't need to provide the It might be more tricky if your invocation of the |
|
Running the |
|
Hi @ktrz , any feedback though? |
|
I'll get back to this during the weekend as I have a pretty busy week |
| @@ -885,6 +886,7 @@ describe('writeBenchmark()', function () { | |||
| externalDataJsonPath: undefined, | |||
| maxItemsInChart: null, | |||
| failThreshold: 2.0, | |||
| commitSha: 'dummy sha', | |||
There was a problem hiding this comment.
I think we should add specific tests for the use-case where we set the explicit commitSha rather than just adding this param here.
Description
Allows the user to pass in a commit SHA to use instead of relying on the payload or head commit. The main use-case of this is for workflow dispatch runs where the SHA is provided through an input.
This change should be valid based on https://docs.github.com/en/rest/reference/commits#get-a-commit.