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

Improve mavenLicenseCheck workflow #372

Closed

Conversation

netomi
Copy link

@netomi netomi commented Sep 9, 2024

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:

OtterdogTest/macos-notarization-service#4

@mbarbero
Copy link

mbarbero commented Sep 9, 2024

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

Copy link
Contributor

@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.

Thanks for all these enhancements, they are all very nice.

But having multiple changes in one commit makes it hard to review this in detail.
Could you be so kind and split this into multiple PRs or at least split the first commit in multiple self contained commits, ideally one per new feature?
I also have the impression that this contains multiple clean-ups (e.g. reorder comments and conditions): Ideally this would be the very first commit together with the withe-space changes.

I'll then try to review this as soon as possible in detail. I'm a bit cautious here since the workflow is used in many projects already and it's annoying for them if it breaks and stressful for us if it has to be fixed quickly.

.github/workflows/mavenLicenseCheck.yml Outdated Show resolved Hide resolved
@netomi
Copy link
Author

netomi commented Sep 10, 2024

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.

@HannesWell
Copy link
Contributor

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 :)

@netomi
Copy link
Author

netomi commented Sep 11, 2024

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
Copy link
Author

netomi commented Sep 13, 2024

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:

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

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
Copy link
Author

netomi commented Sep 13, 2024

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
Copy link
Author

netomi commented Sep 13, 2024

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
Copy link
Author

netomi commented Oct 1, 2024

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

@netomi netomi closed this Oct 1, 2024
@HannesWell
Copy link
Contributor

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. :)

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