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

[CI/CD] detect and prevent unintended third_party repo updates #30746

Closed
andy31415 opened this issue Nov 30, 2023 · 3 comments
Closed

[CI/CD] detect and prevent unintended third_party repo updates #30746

andy31415 opened this issue Nov 30, 2023 · 3 comments
Assignees
Labels
CI/CD improvements Improvements to our CI/CD pipelines good first issue Good for newcomers

Comments

@andy31415
Copy link
Contributor

Due to codegen, developers often do git commit -a to add all changed files, however this also can easily unintentionally change submodues if a they are not synchronized.

Then we have to undo that change (when/if we notice) like in

#30451
#28164

and others. These were the ones that made through, code reviews catch others some of the time, however it is tedious and humans make mistakes.

We should have something that flags PRs that intentionally change submodules and flak (likely failing CI) if unintentional submodules change. Something like a Changing submodules: lwip, gecko_sdk in a comment (I like this because specific) or a label (like changing-submodules-on-purpose) should probably be a requirement to have PRs go in that update submodules.

We also have to make sure dependabot changes go through (configured through https://github.com/project-chip/connectedhomeip/blob/master/.github/dependabot.yml, they create PRs like https://github.com/project-chip/connectedhomeip/pulls?q=is%3Apr+author%3Aapp%2Fdependabot+is%3Aclosed)

@andy31415 andy31415 added the CI/CD improvements Improvements to our CI/CD pipelines label Nov 30, 2023
@kliao-csa kliao-csa assigned kliao-csa and unassigned kliao-csa Dec 6, 2023
@kliao-csa kliao-csa self-assigned this Feb 1, 2024
@vatsalghelani-csa vatsalghelani-csa self-assigned this Apr 11, 2024
@vatsalghelani-csa
Copy link
Contributor

vatsalghelani-csa commented Apr 11, 2024

Either we need a script to detect changes or maybe some rule based on paths changed ... if anything in "/third_party/" is modified we need to run some validation.

https://github.com/project-chip/connectedhomeip/blob/master/.github/workflows/kotlin-style.yaml#L6 - is an example of a workflow that runs for specific path changes only. So we need some logic like:
-> run this workflow when anything under third_party changes (maybe? or maybe .gitmodules is the right path?)
-> when it runs, check that some specific label is applied to the PR - this needs figuring out how.
-> skip running it for dependabot changes

@kliao-csa
Copy link
Contributor

kliao-csa commented Apr 18, 2024

I think what we could try for starters is create a yaml that:

  1. Runs on Pull Request only
  2. Checks specifically for /third_party/ and .gitmodules, so changes to both will trigger it. Note that the third_party filter only handles third party submodules, but gitmodules actually checks for all submodules including ones located elsewhere. I believe this should handle everything we need with the added benefit of blocking unintentional non-submodule additions to third_party
  3. contains( github.event.pull_request.labels.*.name, 'My Label') allows you to check against labels
  4. if: ${{ github.actor != 'dependabot[bot]' }} allows you to exclude dependabot... however we can also use branches-ignore since I don't believe dependabot ever makes pull requests without that branch tag

I can put together an initial script for these requirements this week and have you look over it, Vatsal.

@kliao-csa
Copy link
Contributor

kliao-csa commented Apr 18, 2024

To implement the Changing submodules: lwip, gecko_sdk suggestion, we may use if: contains(github.event.comment.body, 'Changing submodules'. However this institutes a somewhat less intuitive rule that would require exact text input in a comment, so it may be more trouble than it's worth. If we wanted to make sure the submodules listed in the comments actually match the submodule paths changed 1:1, we'd (for the cleanest implementation) need a custom Python script instead of just this workflow. Let's consider how often this kind of issue occurs before going down that path; I believe the changing-submodules-on-purpose label requirement alone will draw enough scrutiny to the pull request to ensure we don't get bad submodule updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD improvements Improvements to our CI/CD pipelines good first issue Good for newcomers
Projects
Archived in project
Development

When branches are created from issues, their pull requests are automatically linked.

4 participants