-
Notifications
You must be signed in to change notification settings - Fork 33
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
Conversation
LGTM, but @HannesWell has the most expertise with this code. It would be great to get your review! |
There was a problem hiding this 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.
Co-authored-by: Hannes Wellmann <[email protected]>
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. |
It would already help if you split those changes that are not inter-winded :)
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 |
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. |
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. |
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. |
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.
If you think there is a great benefit that justifies the extra work I'm fine with it. :) |
This PR makes the following changes to the mavenLicenseCheck.yml workflow:
An example execution can be seen here:
OtterdogTest/macos-notarization-service#4