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

PRs with pending reviews are merged #3661

Closed
akurtakov opened this issue Mar 13, 2024 · 9 comments · Fixed by eclipse/dash-licenses#328
Closed

PRs with pending reviews are merged #3661

akurtakov opened this issue Mar 13, 2024 · 9 comments · Fixed by eclipse/dash-licenses#328

Comments

@akurtakov
Copy link
Member

E.g. #3638 is merged despite licensecheck complaining as it finds out https://github.com/eclipse-tycho/tycho/actions/runs/8253529633/job/22575590947 there is iplab issue is open.

@laeubi
Copy link
Member

laeubi commented Mar 13, 2024

@netomi do you have any hints for us? Actually the ip check should be a required check for the branch protection:

https://github.com/eclipse-tycho/.eclipsefdn/blob/a042ee8257838a90434a5f664b4598266768f777/otterdog/eclipse-tycho.jsonnet#L79

@netomi
Copy link

netomi commented Mar 13, 2024

Looking at the latest data I can see that all checks for the PR passed:

image

The number of required approvals is set to 0, but I guess dependabot is configured to auto-merge if all checks pass. Maybe you wanna disable that.

@laeubi
Copy link
Member

laeubi commented Mar 17, 2024

The automerge is actually desired. I now found the offending commit here:

https://github.com/eclipse-tycho/tycho/actions/runs/8243909268/job/22562813997

it says that the check passed, but the logfile indicates differently @HannesWell could it be that the request for a license (what now works automatically 👍 ) makes the result of the check "green"?

@netomi
Copy link

netomi commented Mar 17, 2024

ah indeed I did not check the license checking workflow. It indeed output some error but does not propagate whether the job failed or not. I suggest to add a final step to the workflow to return 0 if all licenses are vetted or something else if not to be able to use it with branch protection rules and dependabot.

@HannesWell
Copy link
Member

it says that the check passed, but the logfile indicates differently @HannesWell could it be that the request for a license (what now works automatically 👍 ) makes the result of the check "green"?

I have to look it up again since the change of license work-flow to allow auto-request of dependa-bot PR is already some time in the past.
Can you tell why the auto-request works now? I'm not aware of a recent change in the workflow so it must be some configuration change.

@laeubi
Copy link
Member

laeubi commented Mar 17, 2024

I just noticed that the comment seems to be added automatically there also for example:

maybe because github action is now a valid ECA user?

@HannesWell
Copy link
Member

HannesWell commented Mar 17, 2024

maybe because github action is now a valid ECA user?

IIRC the problem that the automated requests of license reviews for dependabot PRs was on the GitHub side since the dependabot user didn't have access to repo/organisation secrets and therefore could not obtain the EF-Gitlab-token.
But if it works now that is great.

Nevertheless, I looked through the workflow and indeed the workflow falsely didn't fail in case of automatically created vetting-requests.
I updated the logic to fix that in eclipse/dash-licenses#328. I'll test it and submit it if it works as desired.

HannesWell added a commit to HannesWell/dash-licenses that referenced this issue Mar 17, 2024
Since eclipse#220 license-reviews
are automatically requested for PRs created by dependabot. But in that
change the logic of the 'Process license check results' step was not
updated to consider that case. This had the effect that the
license-vetting workflow always succeeded for PRs created by dependabot.

The intention to not fail the workflow in general when a license-review
was requested is because initially review-request were only created
manually from committers by adding a corresponding comment to the PR.
And that workflow execution should not fail respectively its result was
actually irrelevant.

Fixes eclipse-tycho/tycho#3661
HannesWell added a commit to HannesWell/dash-licenses that referenced this issue Mar 17, 2024
Since eclipse#220 license-reviews
are automatically requested for PRs created by dependabot. But in that
change the logic of the 'Process license check results' step was not
updated to consider that case. This had the effect that the
license-vetting workflow always succeeded for PRs created by dependabot.

The intention to not fail the workflow in general when a license-review
was requested is because initially review-request were only created
manually from committers by adding a corresponding comment to the PR.
And that workflow execution should not fail respectively its result was
actually irrelevant.

Fixes eclipse-tycho/tycho#3661
HannesWell added a commit to eclipse/dash-licenses that referenced this issue Mar 17, 2024
Since #220 license-reviews
are automatically requested for PRs created by dependabot. But in that
change the logic of the 'Process license check results' step was not
updated to consider that case. This had the effect that the
license-vetting workflow always succeeded for PRs created by dependabot.

The intention to not fail the workflow in general when a license-review
was requested is because initially review-request were only created
manually from committers by adding a corresponding comment to the PR.
And that workflow execution should not fail respectively its result was
actually irrelevant.

Fixes eclipse-tycho/tycho#3661
@HannesWell
Copy link
Member

Nevertheless, I looked through the workflow and indeed the workflow false failed in case of automatically created vetting-requests.
I updated the logic to fix that in eclipse/dash-licenses#328. I'll test it and submit it if it works as desired.

All testing was successful and the PR is submitted. This issue should now be fixed.

@laeubi
Copy link
Member

laeubi commented Mar 17, 2024

Thank you! This will further simplify things when upgrading!

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 a pull request may close this issue.

4 participants