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

feat: implement requiredStatusChecks for GitHub and GitLab #4597

Closed
wants to merge 2 commits into from

Conversation

sarunint
Copy link
Contributor

@sarunint sarunint commented Oct 7, 2019

Closes #1853 (partially)

@sarunint sarunint force-pushed the requiredStatusChecks-gh branch from 37a8091 to 8266281 Compare October 7, 2019 08:33
@sarunint sarunint closed this Oct 7, 2019
@sarunint sarunint reopened this Oct 7, 2019
@rarkins
Copy link
Collaborator

rarkins commented Oct 7, 2019

There are some obvious use cases here but also some less obvious.

  1. If all required status checks are present and all passing: succeeded
  2. If any required status checks are present and failed: failed
  3. If only some of the status checks are present but all that are present are passing: pending?

The key question is: what to do if the user configures a required status check that fails to show up? I think we should probably be pending forever. What do you think?

@sarunint
Copy link
Contributor Author

sarunint commented Oct 7, 2019

Ha, I totally missed those cases.

@sarunint
Copy link
Contributor Author

sarunint commented Oct 7, 2019

Let me think for a while.

Also, should I do this for other platforms in this PR? Or should I separate them up?

@rarkins
Copy link
Collaborator

rarkins commented Oct 7, 2019

Ideally we would implement this for all platforms that could support it. It's OK to note a feature is "platform X only" but it can confuse users even still, so ideally we don't do that.

@sarunint sarunint force-pushed the requiredStatusChecks-gh branch 3 times, most recently from e9e1e0a to 187073e Compare October 7, 2019 11:20
@sarunint sarunint changed the title feat(github): implement requiredStatusChecks WIP: feat(github): implement requiredStatusChecks Oct 7, 2019
@sarunint sarunint force-pushed the requiredStatusChecks-gh branch 3 times, most recently from 5c8782c to 6d4c552 Compare October 7, 2019 12:43
@sarunint sarunint force-pushed the requiredStatusChecks-gh branch from 6d4c552 to dd334fb Compare October 7, 2019 12:48
@sarunint sarunint changed the title WIP: feat(github): implement requiredStatusChecks feat: implement requiredStatusChecks for GitHub and GitLab Oct 7, 2019
@sarunint
Copy link
Contributor Author

sarunint commented Oct 7, 2019

@rarkins I ended up implement for both GitHub and GitLab.

@rarkins rarkins requested a review from viceice October 8, 2019 06:58
@rarkins
Copy link
Collaborator

rarkins commented Oct 8, 2019

@leipert could you glance at the GitLab part of this?

Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

Can we extract some util function for this? I see a lot duplicated code.

if (
commitStatus.state === 'failed' ||
filteredStatusesCombined === 'failed' ||
checkRuns.some(run => run.conclusion === 'failed')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we be applying the same logic to check runs? What happens if the user applies requiredStatusChecks while also there are some check runs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll see what I can do.

Copy link
Collaborator

@rarkins rarkins left a comment

Choose a reason for hiding this comment

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

Check runs (github) support. Also, possible to de-duplicate the core logic between github and gitlab implementations?

@viceice
Copy link
Member

viceice commented Nov 29, 2019

@sarunint any news? Are you planning any changes?

@viceice
Copy link
Member

viceice commented Nov 29, 2019

This should be simple adaptable for bitbucket and bitbucket-server.

azure would be more difficult.

const branch = await azureApiGit.getBranch(

branch.commit.statuses

https://docs.microsoft.com/en-us/rest/api/azure/devops/git/statuses/list?view=azure-devops-rest-5.1
https://docs.microsoft.com/en-us/rest/api/azure/devops/git/pull%20request%20statuses/list?view=azure-devops-rest-5.1

@sarunint
Copy link
Contributor Author

@viceice Sorry, I've been very busy this month. I'll try to fix things up this weekend.

Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

Needs deconflicting

@CLAassistant
Copy link

CLAassistant commented Mar 3, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@rarkins rarkins marked this pull request as draft May 30, 2020 05:18
@rarkins rarkins closed this Aug 5, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants