dash-licenses icon indicating copy to clipboard operation
dash-licenses copied to clipboard

Improve mavenLicenseCheck workflow

Open netomi opened this issue 1 year ago • 7 comments

This PR makes the following changes to the mavenLicenseCheck.yml workflow:

  • parse the output of the license check execution and add it as a PR comment (based on the version of the eclipse-set project)
  • also attach the same output as JOB summary: this is a workaround for the cases were PRs from forks are not able to add a comment to the PR, people are able to go to the associated workflow run and see the same output in the job summary
  • additionally, the job summary change could be used to also handle PRs from forks: define a workflow that listens for workflow_run completed events and then attaches the job summary as comment to the associated PR. This could also be automated in our self-service otterdog so that no additional workflow would be needed. If you like the idea I can certainly come up with a demo. We did something similar already for other use-cases (attach code coverage report as a PR comment on completion of another workflow).
  • support failure states for the license-check workflow to be able to use it as a status-check for branch protection rules, the workflow will now fail if not all licenses are vetted
  • support rerunning the license-check by adding a comment /license-check as a comment, this will trigger a rerun of the original workflow run to be compatible with the way GitHub treats workflows runs as checks (only if the are triggered from pull_requests they are considered)

An example execution can be seen here:

https://github.com/OtterdogTest/macos-notarization-service/pull/4

netomi avatar Sep 09 '24 14:09 netomi

LGTM, but @HannesWell has the most expertise with this code. It would be great to get your review!

mbarbero avatar Sep 09 '24 19:09 mbarbero

I understand your concern, however I am not sure I can really extract the different changes in separate commits, some of them are intertwined.

However, what do you think about creating a new repo like dash-workflows, that adds these actions and workflows. That would allows for a fresh start and make it easier to version the workflows. The current practice to reference @master is certainly not a good practice and a recipe for failure.

netomi avatar Sep 10 '24 06:09 netomi

I understand your concern, however I am not sure I can really extract the different changes in separate commits, some of them are intertwined.

It would already help if you split those changes that are not inter-winded :) But if that's not possible I will review it as it is.

However, what do you think about creating a new repo like dash-workflows, that adds these actions and workflows. That would allows for a fresh start and make it easier to version the workflows. The current practice to reference @master is certainly not a good practice and a recipe for failure.

In general I have no objection, it's just that I'm not sure if it's worth the overhead. Using tags seems a good idea, but we could also do this in this repo can't we? If the general tags like 1.x or v1.x are intended for the main code, we could use other tags for the the workflow: E.g. w1.x, where the w stands for workflow. This would also not break existing references and would allow adjustments in the workflow together with code changes if necessary. But both is nothing that is absolute important. The latter probably less than the form. But we can hopefully reach all existing users through the cross-project mailing list if necessary. But if you think using a separate repo makes things much better I'm also fine with it :)

HannesWell avatar Sep 10 '24 22:09 HannesWell

I think having different version tags for different content in a repo is highly confusing. Also there is a convention wrt versioning of GitHub Actions that we should follow, so thats why I brought up the idea of a separate repo as in the dash-licenses repo it will not be possible and is not desirable as the dash tool itself follows a different version scheme and we should not try to mix them imho.

I will work today a bit on the PR to have separate commits and also add another workflow to add the comments for PRs from forks.

netomi avatar Sep 11 '24 07:09 netomi

Updated the PR to include a reusable workflow to add comments also to PRs coming from a fork.

My first idea was to reuse the job summary, but apparently GitHub has changed something and the undocumented way to access the job summary does not work anymore.

So as a workaround, we do what GitHub suggests by themselves, store the comment and pr number as an artifact of the job and download it later in a second workflow run.

You can see an example here:

  • https://github.com/OtterdogTest/macos-notarization-service/pull/6

The following workflow needs to be added to the repo to support that:

  • https://github.com/OtterdogTest/macos-notarization-service/blob/main/.github/workflows/add-job-summary-as-comment.yml

If such a workflow is missing, the comment will not be added but but everything else will work normally and not result in an error.

If you agree on something like that I can add it also to the README.

netomi avatar Sep 13 '24 12:09 netomi

For reference:

I was trying to access the job summary as described here: https://stackoverflow.com/questions/76145922/how-to-get-github-actions-workflow-summary

That seems to have worked for some times, as there are also actions available in the marketplace doing exactly that, but it does not work anymore. The now implemented option is working as it uses public API calls that are not going to change.

netomi avatar Sep 13 '24 12:09 netomi

the license summary that is displayed contains now also a valid link to the ticket in the IPLab project:

image

pinned all actions and fixed the owner for the called workflow from netomi to eclipse-dash

netomi avatar Sep 13 '24 12:09 netomi

The PR is probably to complex, will think about other ways to support that.

netomi avatar Oct 01 '24 18:10 netomi

The PR is probably to complex, will think about other ways to support that.

Sorry I lost track of this since it's currently quite busy for me. Having this in smaller steps as far as possible (either in multiple commits in one PR or multiple PRs) would really help to review.

Just from looking at the result the comment looks nice.

Also there is a convention wrt versioning of GitHub Actions that we should follow, so thats why I brought up the idea of a separate repo as in the dash-licenses repo it will not be possible and is not desirable as the dash tool itself follows a different version scheme and we should not try to mix them imho.

If you think there is a great benefit that justifies the extra work I'm fine with it. :)

HannesWell avatar Oct 01 '24 18:10 HannesWell