Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: persist inputs between the upload action and its post step #2557

Merged
merged 6 commits into from
Oct 21, 2024

Conversation

NlightNFotis
Copy link
Member

@NlightNFotis NlightNFotis commented Oct 21, 2024

Description

This PR is fixing an issue with persisting action inputs between steps of the runner. This is an issue that is downstream of a problem with the GitHub Actions runner, ref: actions/runner#3514

Closes #2553

Merge / deployment checklist

Acknowledgments

  • Thanks to @chrisgavin for the code that formed the basis of this PR, and @jsoref for the original issue report, a PR containing code leading us to a solution, and for helping us iterate on this PR by testing our changes and proposing further improvements.

@NlightNFotis NlightNFotis requested a review from a team as a code owner October 21, 2024 10:09
Co-authored-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
@NlightNFotis NlightNFotis changed the title Persist inputs between teh upload action and its post step. fix: persist inputs between the upload action and its post step Oct 21, 2024
Copy link
Contributor

@jsoref jsoref left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Tested it https://github.com/check-spelling-sandbox/check-spelling/actions/runs/11439466596/job/31823193138 and it worked. (The patch partially failed to apply because the source map wasn't a match to the version included in the runner cache, but that isn't a requirement -- and when I was doing my testing, I didn't include the source map file.)

Note that there are four actions that use post:. If I were maintaining this repository, I'd patch all of them otherwise, there's a risk that someone else gets tripped up that this only works in some code paths...

@jsoref
Copy link
Contributor

jsoref commented Oct 21, 2024

@NlightNFotis can you cherry-pick d9886b8 ? VSCode automatically wanted to remove that whitespace...

src/analyze-action-post.ts Fixed Show fixed Hide fixed
src/init-action-post.ts Fixed Show fixed Hide fixed
src/init-action-post.ts Fixed Show fixed Hide fixed
@NlightNFotis
Copy link
Member Author

@NlightNFotis can you cherry-pick d9886b8 ? VSCode automatically wanted to remove that whitespace...

Hm, I want to do that, but I get

❯ git cherry-pick d9886b85848afc58f19094c760535a6dd1a8cf77
fatal: bad object d9886b85848afc58f19094c760535a6dd1a8cf77

I think the original branch with this change has been deleted, so it cannot find the commit. I also looked into your fork and I cannot find the branch there.

Would it be possible to extract it as a patch and post it here as a comment? @jsoref

@jsoref
Copy link
Contributor

jsoref commented Oct 21, 2024

You can grab it from https://github.com/github/codeql-action/commit/d9886b85848afc58f19094c760535a6dd1a8cf77.patch

Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
@NlightNFotis
Copy link
Member Author

@jsoref Done! Thanks again for your time and effort helping us get to a good state in our code! Your input is greatly appreciated.

Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure whether this is happening due to a bug or an intended omission in the mechanisms for passing inputs, but the fix LGTM.

CHANGELOG.md Outdated Show resolved Hide resolved
src/actions-util.ts Outdated Show resolved Hide resolved
src/analyze-action.ts Outdated Show resolved Hide resolved
src/init-action-post.ts Outdated Show resolved Hide resolved
src/init-action.ts Outdated Show resolved Hide resolved
src/start-proxy-action-post.ts Outdated Show resolved Hide resolved
src/start-proxy-action.ts Outdated Show resolved Hide resolved
src/upload-sarif-action.ts Show resolved Hide resolved
src/upload-sarif-action-post.ts Show resolved Hide resolved
…toreState callsites.

Co-authored-by: Henry Mercer <henrymercer@github.com>
Co-authored-by: Fotis Koutoulakis <nlightnfotis@github.com>
@NlightNFotis
Copy link
Member Author

@henrymercer Thank you for the review, I have integrated all of the changes in 9bc4ee1. Would you please have another look at this one?

Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NlightNFotis NlightNFotis merged commit b15a5b5 into main Oct 21, 2024
275 checks passed
@NlightNFotis NlightNFotis deleted the persist-inputs branch October 21, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

upload-sarif post-action step failed: Input required and not supplied: token
4 participants