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

Closes #17: Added 'mergeable' check to cover cases where PRs don't have checks #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BravoNatalie
Copy link

@BravoNatalie BravoNatalie commented Nov 10, 2022

The current implementation of the stateQuery fails if the PR does not have any checks associated with it but can be merged. To solve this, the mergeable property was added to the stateQuery and it's evaluated in the case of statusCheckRollup being null.

This PR closes the issue #17 .

@BravoNatalie BravoNatalie changed the title Added 'mergeable' check to cover cases where PRs don't have checks Closes #17: Added 'mergeable' check to cover cases where PRs don't have checks Nov 10, 2022
AloisSeckar added a commit to AloisSeckar/ELRHistory that referenced this pull request Feb 16, 2023
@martingjaldbaek
Copy link
Member

Hi @BravoNatalie - thanks for the PR!

And sorry this took some time to look at - first off, life happened. But secondly I also wasn't sure if this was strictly an improvement, or if there was a risk of causing problems/confusion, so I've been mulling this over. Here are the issues I've been considering:

  1. There is already the option of setting "mustBeGreen" to false, as a way to work with PRs that don't get checks run on them
    • (Judging by the number of likes on this PR, I think I might need to highlight that more in the documentation, or have "mustBeGreen" default to false, to help those that don't have time to figure out what all the parameters mean)
  2. Adding a second alternative interpretation of "mustBeGreen" - that it automatically falls back to - runs the risk of having mis-configurations of checks go unnoticed. I'm wondering if instead of a boolean for mustBeGreen, we should have an enum - e.g. "requirements" that could be either "None", "Green" or "Mergeable", so the user can explicitly opt into the behavior they want?
  3. PRs with UNKNOWN/not-yet-defined mergeability will be left out of the combined PR. In my tests, if I had recently made any change to the base branch, then all PRs had status UNKNOWN and were left out. If I view any specific PR on GitHub, this nudges GitHub to update its mergeability status again, and it gets included in subsequent runs of the workflow.
    • I feel like this is quite a "gotcha" that users probably won't expect. Part of the point of this workflow is to avoid manual process. Is there a way we can nudge GitHub to update the mergeability status, and wait for that to complete, during the workflow? If not, we should at least consider listing the PRs that are left out of the Combined PR due to unkown mergeability, like we currently do with those left out due to merge conflicts.
  4. The are some unrelated linter changes in the PR. After mulling it over, I'm ok with using double-quoted strings, so whatever

Overall, if it wasn't for 3) I would have merged this already. But I do worry that having the workflow be able to silently fail for non-obvious reasons is a bit of a foot-gun for users. Any thoughts?

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.

2 participants