Skip to content

Revert "Start waiting for processing by default." #878

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.

Sign up for GitHub

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

Jump to bottom
Merged
edoardopirovano merged 2 commits into from
Jan 21, 2022
Merged

Revert "Start waiting for processing by default." #878

edoardopirovano merged 2 commits into from
Jan 21, 2022

Conversation

@chrisgavin
Copy link
Contributor

@chrisgavin chrisgavin commented Jan 21, 2022

This reverts commit b661ef1.

It looks like this change caused issues with analyses on pull requests from forks. This is due to the complicated nature of the permissions of workflows run on these pull requests. They are able to upload analyses for their own pull requests refs, but are not able to access other Code Scanning APIs such as the one required to get the status of a delivery.

This reverts adding this new functionality until we update the API to make it able to safely determine whether a pull request should be able to get the status of a particular delivery.

This is a hotfix on the v1 branch.

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 requested a review from cannist January 21, 2022 10:04
@chrisgavin chrisgavin requested a review from a team as a code owner January 21, 2022 10:04
@edoardopirovano
Copy link
Contributor

edoardopirovano commented Jan 21, 2022

Could we add a changelog note mentioning this revert? We mentioned the original change in the log, so I think the revert should be too.

@chrisgavin
Copy link
Contributor Author

chrisgavin commented Jan 21, 2022

Sure! I've put this under a 1.0.29 release with today's date on. Let me know if this is not the correct way of doing a change note for a hotfix though.

@edoardopirovano
Copy link
Contributor

edoardopirovano commented Jan 21, 2022

Sure! I've put this under a 1.0.29 release with today's date on. Let me know if this is not the correct way of doing a change note for a hotfix though.

Oh I've just realized this PR is targeting v1. I think it should probably target main and the note should go in the "unreleased" heading. Then our automation can handle doing a release with the fix in and do the right things with regards to bumping the version number in all the right places.

@chrisgavin chrisgavin changed the base branch from v1 to main January 21, 2022 10:25
@chrisgavin chrisgavin force-pushed the revert-wait-for-processing branch from 7a196e8 to 7ec25e0 Compare January 21, 2022 10:26
@chrisgavin
Copy link
Contributor Author

chrisgavin commented Jan 21, 2022

Ah, okay that makes sense. I was a bit worried that would make it more difficult to do a hotfix because it would pull in other unrelated changes, but it actually looks like there aren't any, so that should work fine.

@edoardopirovano
Copy link
Contributor

edoardopirovano commented Jan 21, 2022

Ah, okay that makes sense. I was a bit worried that would make it more difficult to do a hotfix because it would pull in other unrelated changes, but it actually looks like there aren't any, so that should work fine.

Yeah, if the day ever comes that we have to do a hotfix directly on v1 it will be a slight pain because we'll need to manually bump the version number in a few places then do a manual mergeback to main. No need for that here though, much easier to do the change on main and let the automation handle releasing.

@edoardopirovano edoardopirovano merged commit 67c0353 into main Jan 21, 2022
@edoardopirovano edoardopirovano deleted the revert-wait-for-processing branch January 21, 2022 10:45
@cannist
Copy link
Contributor

cannist commented Jan 21, 2022

👍

@github-actions github-actions bot mentioned this pull request Jan 21, 2022
Merged
5 tasks
@henrymercer henrymercer mentioned this pull request Jan 21, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cannist cannist Awaiting requested review from cannist

1 more reviewer

@edoardopirovano edoardopirovano edoardopirovano approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@chrisgavin @edoardopirovano @cannist