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

Re-enable waiting for processing by default, using the new API semantics. #1007

Merged
merged 2 commits into from Apr 14, 2022

Conversation

chrisgavin
Copy link
Contributor

This re-enables waiting for processing by default, after some changes to the API which will allow it to work on pull requests from forks.

It also makes it more tolerant of errors returned by the API, since waiting for processing is not essential.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@chrisgavin chrisgavin marked this pull request as ready for review March 30, 2022 12:23
@chrisgavin chrisgavin requested a review from a team as a code owner March 30, 2022 12:23
@edoardopirovano
Copy link
Contributor

Would you mind if we hold this until next week when we've fixed the SHA of codeql-action that goes into GHES 3.5?

@edoardopirovano
Copy link
Contributor

A thought I had just after approving: There was previously an issue with the API end-point that has now been fixed, right? But does that mean this could cause problems for users on old versions of GHES who pull in a new version of the Action? Should we gate this to only work on dot com or on new versions of GHES?

@edoardopirovano
Copy link
Contributor

I appreciate the switch to a warning means we won't fail, but it would still be nice to avoid warnings in the log.

@chrisgavin chrisgavin merged commit c5c5bda into main Apr 14, 2022
@chrisgavin chrisgavin deleted the wait-for-processing-2 branch April 14, 2022 08:29
@chrisgavin
Copy link
Contributor Author

So you will still get a warning on old GHES versions, that is true. When I was implementing this it seemed somewhat reasonable to me, because it is indeed true that we cannot tell you if your analysis processing will fail.

I can add a check for the GHES version if you think this is not the best behavior though.

@edoardopirovano
Copy link
Contributor

Hmm, I guess it's fine to have a warning in the logs in the fairly rare case of a pull request from a fork in a GHES instance. Or at least, it's probably not worth the extra complexity of having version checks in the code. Let's leave this as-is.

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

3 participants