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

MNT Check licenses #83

Merged

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Dec 19, 2024

Issue #80

Cron will run on the default branch, which is currently 5. We'll need to manually run the job targetting different branch e.g. 6.0 to check the licenses there

@emteknetnz emteknetnz force-pushed the pulls/5/license-checker branch from 9b925bf to ddfd3ad Compare December 19, 2024 07:06
@emteknetnz emteknetnz marked this pull request as ready for review December 19, 2024 07:37
@GuySartorelli
Copy link
Member

We'll need to manually run the job targeting different branch e.g. 6.0 to check the licenses there

Can we leverage GitHub Actions dispatch API to do this for us? I'm not fussed either way but seems worth considering at least.

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

On the whole the code itself looks good. Is there a CI run on your own account or somewhere that I can see this running?

.github/workflows/licenses.yml Outdated Show resolved Hide resolved
.github/workflows/licenses.yml Outdated Show resolved Hide resolved
@emteknetnz
Copy link
Member Author

emteknetnz commented Dec 19, 2024

My own account, though you probably don't have access to view the logs - https://github.com/emteknetnz/recipe-kitchen-sink/actions/runs/12407915243

Ran in over 13 minutes (bit more than the 1 or 2 I was expecting), went green, a few seconds for composer and just under 13 minutes for npm

@emteknetnz emteknetnz force-pushed the pulls/5/license-checker branch from ddfd3ad to 5e9b7e5 Compare December 19, 2024 21:33
@GuySartorelli
Copy link
Member

I'm seeing lots of

Error: No packages found in this path..

Does that mean it's failing to check some of our dependencies?

Please also respond to #83 (comment)

@emteknetnz
Copy link
Member Author

Error: No packages found in this path.. means there are no non-dev dependencies. It's kind of a short-coming of the npm license-checker

Since that's confusing, I'll update the script to validate there are non-dev deps before running it

@emteknetnz
Copy link
Member Author

emteknetnz commented Dec 19, 2024

Can we leverage GitHub Actions dispatch API to do this for us? I'm not fussed either way but seems worth considering at least.

Yes considered that, though to match the pattern used for gha-dispatch-ci, that would require creating a whole new silverstripe/gha-dispatch-liceneses and silverstripe/gha-liceneses repos with actions, which just seems excessive

Would probably get away with not having one of those repos (probably gha-dispatch-licenses), though still seems a bit wrong to have a standalone action that only runs on a single module

I think we do need at least one extra repo is because github actions doesn't want to run things on a cron that aren't the default branch

@GuySartorelli
Copy link
Member

I don't think we'd need entirely new repos for it - but it's not much effort to click the buttons once a week so I'm okay without using dispatch for this.

@emteknetnz emteknetnz force-pushed the pulls/5/license-checker branch from 5e9b7e5 to 56e5e08 Compare December 19, 2024 22:23
@emteknetnz
Copy link
Member Author

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

LGTM

@GuySartorelli
Copy link
Member

Not waiting for CI since this won't affect it - any failures will be unrelated anyway.

@GuySartorelli GuySartorelli merged commit 3edd419 into silverstripe:5 Dec 19, 2024
57 of 58 checks passed
@GuySartorelli GuySartorelli deleted the pulls/5/license-checker branch December 19, 2024 22:48
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.

2 participants