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 README error #392

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix README error #392

wants to merge 3 commits into from

Conversation

dslatkin
Copy link

@dslatkin dslatkin commented Jun 2, 2023

I was using the example from the README to setup an async script in a workflow and got this error from the API:

Unhandled error: HttpError: No commit found for SHA: undefined

Digging a bit deeper, I discovered that process.env.SHA didn't exist, and should probably be GITHUB_SHA instead. Swapping it out to this worked.

@dslatkin dslatkin requested a review from a team as a code owner June 2, 2023 02:38
Copy link
Member

@joshmgross joshmgross left a comment

Choose a reason for hiding this comment

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

I believe this example was intending the SHA to be set as an input

      - uses: actions/github-script@v6
        env:
          SHA: '${{env.parentSHA}}'
        with:
          script: |
            const script = require('./path/to/script.js')
            await script({github, context, core})

So process.env.SHA wasn't meant to directly match an existing variable in the context like GITHUB_SHA

@dslatkin
Copy link
Author

That makes sense. I'm not sure I see parentSHA defined anywhere, either. Would it make sense to remove lines 341-342 in that case as well to reduce confusion? Thank you for reviewing!

@dslatkin dslatkin requested a review from a team as a code owner November 28, 2023 20:44
@dslatkin
Copy link
Author

That makes sense. I'm not sure I see parentSHA defined anywhere, either. Would it make sense to remove lines 341-342 in that case as well to reduce confusion? Thank you for reviewing!

Just a heads up, I did just remove these lines in a new commit.

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.

None yet

2 participants