Skip to content

Conversation

@motatoes
Copy link
Contributor

@motatoes motatoes commented May 23, 2023

Unstable means: "Mergeable with non-passing commit status." So it should be in the map of mergeable states

https://docs.github.com/en/github-ae@latest/graphql/reference/enums#mergestatestatus

@fleroux514
Copy link
Contributor

@motatoes As I understand it, this status would mean that the CI fails somehow. And with the current logic that would mean that when auto_merge is set to true and the status is Unstable then the PR could possibly get merged, which is probably not wanted. Am I mistaken?

@fleroux514
Copy link
Contributor

I'm trying to lookup a use-case that would result with this status but its a bit obscure. Even chat-GPT doesn't seem to know about it :).

Did you have a specific scenario in mind regarding this fix? In doubt, I personally would prefer having a more strict behavior for now and add unstable when we can verify that there is indeed a bug and its needed, then the other way around that might result in and unwanted PR merge when running apply.

The function isMergeableState() prints the status at line 98 when returning false. And in the worst case, users could merge the PR manually if needed after running apply.

@motatoes
Copy link
Contributor Author

hey @fleroux514 thanks for looking into this I did run into this while setting up digger on a private repo, although all the checks were successful the digger apply command failed with the error "PR is not in a mergable state" while the merge button was active and I could infact merge it. However the apply was no longer possible. I am going to attempt reproducing this in a public repo

@motatoes
Copy link
Contributor Author

motatoes commented May 23, 2023

@fleroux514 here is a reproduction of the issue

pull request: motatoes/digger-test-mergeable#1

action in unstable state: https://github.com/motatoes/digger-test-mergeable/actions/runs/5059997314

you will see that the pull request has 1 commit which fails plan due to invalid terraform, now the next commit has succeeded te terraform plan phase but I am now unable to apply due to it being in an "unstable" state (although its perfectly mergeable)

@fleroux514
Copy link
Contributor

fleroux514 commented May 24, 2023

Thanks @motatoes. I find it to be a weird behavior from Github but based on this test I think we can proceed with this change.

However it is not the reason why digger apply is not working. It should only affect if the PR gets merged or not (the apply command is supposed to be executed before I believe)

@Spartakovic Spartakovic merged commit de3436d into main May 24, 2023
@frank-bee
Copy link

@fleroux514 here is a reproduction of the issue

pull request: motatoes/digger-test-mergeable#1

action in unstable state: https://github.com/motatoes/digger-test-mergeable/actions/runs/5059997314

you will see that the pull request has 1 commit which fails plan due to invalid terraform, now the next commit has succeeded te terraform plan phase but I am now unable to apply due to it being in an "unstable" state (although its perfectly mergeable)

FYI : I also have this issue ( we are working it github.com enterprise with private repos)

@frank-bee
Copy link

I tried this again with

 - name: digger run
        uses: diggerhq/digger@main"

Still having the issue

@frank-bee
Copy link

... sorry , I see , that the github action with this fix is not released yet. got it

@motatoes
Copy link
Contributor Author

hey @frank-bee it should be fixed in @v0.1.21 :)

ben-of-codecraft pushed a commit to ben-of-codecraft/digger that referenced this pull request May 21, 2024
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.

5 participants