8000 [CI] Make sure packages contain all metafiles by Nyholm · Pull Request #40700 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[CI] Make sure packages contain all metafiles #40700

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

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 8, 2021
Merged

[CI] Make sure packages contain all metafiles #40700

merged 1 commit into from
Apr 8, 2021

Conversation

Nyholm
Copy link
Member
@Nyholm Nyholm commented Apr 3, 2021 •
edited
Loading
Q A
Branch? 5.x
Bug fix? no
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

This PR introduce a new CI job to just do some checks on a package. It first looks at the git diff to figure out what packages have changed. Then for each changed package it verifies:

  • .gitattributes exists
  • .gitignore exists
  • CHANGELOG.md exists
  • LICENSE exists
  • phpunit.xml.dist exists
  • README.md exists
  • The LICENSE file includes "Fabien Potencier", the correct year and has same content as /LICENSE

If the package is new (license file changed), we also make sure that the package is present in composer.json's replace.

I got this idea after seeing PRs like #40696. And we do add one or two package every month.

@Nyholm Nyholm force-pushed the meta-files branch 2 times, most recently from 47e3709 to edd93cf Compare April 3, 2021 14:35
@nicolas-grekas
Copy link
Member

I think I'd prefer making this part of an existing CI file. I don't like the multiplication of eg initialization steps. That'd be better for the planet... :) Doable?

@OskarStark
Copy link
Contributor

Nice 😀😃

@nicolas-grekas
Copy link
Member

About getting the modified packages, maybe it's not worth it? maintaining these is always costly on the long run.

@ro0NL
Copy link
Contributor
ro0NL commented Apr 3, 2021

find src/ -mindepth 3 -maxdepth 3 -type d '!' -exec test -e "{}/LICENSE" ';' -print minus the false positives?

@Nyholm
Copy link
Member Author
Nyholm commented Apr 5, 2021

Thank you for the feedback.

Instead of having one job per changed component +1 I now do all the checks in a single job. It means a bit more bash and we have to deal with json objects.

I do some grouping and I show clear errors what is wrong.

@Nyholm Nyholm force-pushed the meta-files branch 3 times, most recently from 01597a1 to a99061e Compare April 5, 2021 09:15
@Nyholm Nyholm mentioned this pull request Apr 7, 2021
Merged
@derrabus
Copy link
Member
derrabus commented Apr 7, 2021

I think this job is a good idea. It's a common mistake to forget adding these files when contributing a new bridge package.

@Nyholm
Copy link
Member Author
Nyholm commented Apr 7, 2021

I removed the debug code. This PR is ready to be merged.

@fabpot
Copy link
Member
fabpot commented Apr 8, 2021

Thank you @Nyholm.

@fabpot fabpot merged commit 790e2a8 into symfony:5.x Apr 8, 2021
@Nyholm
Copy link
Member Author
Nyholm commented Apr 8, 2021

Thank you for merging

@Nyholm Nyholm deleted the meta-files branch April 11, 2021 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@fabpot fabpot fabpot approved these changes

@derrabus derrabus derrabus approved these changes

Assignees
No one assigned
Labels
Status: Reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Nyholm @nicolas-grekas @OskarStark @ro0NL @derrabus @fabpot @stof @carsonbot
0