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

Automatically check licenses and request review for target updates #2760

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

Conversation

laeubi
Copy link
Contributor

@laeubi laeubi commented Jan 19, 2025

If there are updates to the target contents there is some chance we need a review.

This now adds a job that when a PR is created or updated calls the license check workflow together with a review request.

@laeubi laeubi force-pushed the request_licences_review_always branch 2 times, most recently from a664f91 to abfeda5 Compare January 19, 2025 07:37
Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

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

Calling the maven-license-check-action will trigger a review of all not vetted artifacts, that looks good.
But you wont get a comment with the pretty-printed results added to the PR created, which I think is especially useful if some artifacts are not vetted and one wants to check the requests status.

With the current dash-licenses workflow, this would require the platform-bot to add the usual /request-license-review. This seems a bit cumbersome on first sight, but eventually it's just another event-trigger, like adding a label or alike.

@laeubi
Copy link
Contributor Author

laeubi commented Jan 19, 2025

Yes this will be "silent" at the moment, but my idea is that if one sometimes later rerun the check it is already vetted (so it is actually good), maybe one can make it add a label to the PR (e.g vetted or requires vetting...) as we are all getting already to much mails I think.

@HannesWell
Copy link
Member

Yes this will be "silent" at the moment, but my idea is that if one sometimes later rerun the check it is already vetted (so it is actually good),

Yes, it's definitely better than before.

maybe one can make it add a label to the PR (e.g vetted or requires vetting...) as we are all getting already to much mails I think.

Yes there are way to many mails, but luckily my mail-program sorts the mails into threads and deleting a two or three mail thread is not really a difference for me 😅. Nevertheless running the license-workflow if a label is present is probably best implemented by generalizing the case for 'automated requests for dependabot PRs' (which unfortunately doesn't work currently due to the permissions model):
https://github.com/eclipse-dash/dash-licenses/blob/9b1616198579426e36cc81f8fde2fffcc65d6103/.github/workflows/mavenLicenseCheck.yml#L61-L66

@laeubi
Copy link
Contributor Author

laeubi commented Jan 19, 2025

By label I mean, that this job can add a label to the PR when the check was successful, so one can "see" it... but thinking further now one better would want to rerun the license check check if vetting was performed... but this will require a lot more work I fear.

@HannesWell
Copy link
Member

By label I mean, that this job can add a label to the PR when the check was successful, so one can "see" it... but thinking further now one better would want to rerun the license check check if vetting was performed... but this will require a lot more work I fear.

That would be nice, if possible I would even add a comment to ping the the person responsible for the PR, but as you said that's much more work as it requires a connection from the EF gitlab to the corresponding GH PR. But first of all it would require eclipse-dash/dash-licenses#184, because currently there is no direct link from a Gitlab IP issue to the creating PR.

@akurtakov
Copy link
Member

Is there a reason to not push this one now? Silent request is still better than manual one IMO.

@laeubi
Copy link
Contributor Author

laeubi commented Mar 3, 2025

From my side we can try it out ...

If there are updates to the target contents there is some chance we need
a review.

This now adds a job that when a PR is created or updated calls the
license check workflow together with a review request.
@akurtakov akurtakov force-pushed the request_licences_review_always branch from abfeda5 to 1f8eea4 Compare March 3, 2025 07:40
@laeubi
Copy link
Contributor Author

laeubi commented Mar 3, 2025

@akurtakov but We would need a new parameter and a condition check here as it is now a shared workflow.

@laeubi laeubi marked this pull request as draft March 3, 2025 07:45
@HannesWell
Copy link
Member

But you wont get a comment with the pretty-printed results added to the PR created, which I think is especially useful if some artifacts are not vetted and one wants to check the requests status.

I still think not having a comment that adds the created reviews is really sub-optimal as one then has to search the build-logs.
A while ago I have adjusted the workflow to request a review automatically for dependabot PRs:

and it seems to work well:

I think this pattern can be extended to either add an option to permit more bot/user accounts or to add an option that for PRs of a committer (i.e. actors with write access) a review is requested automatically (and the bot should have write access).

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.

3 participants