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

Limit maximum number of lines changed in file #1468

Open
wants to merge 3 commits into
base: main
from
Open
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 70 additions & 0 deletions .github/workflows/limit-pr-files-changed.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
name: Enforce Max Lines Changed Per File

on:
pull_request:
paths:
- '**/*.java' # Monitor changes to Java files only
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also include .avsc and .proto files please?

Schema changes ideally deserve dedicated PRs anyway, but without making things too complicated at first, we could at least include them in the count?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But for new protocol updates, 'added' will count the entire file as added which would be very high most of the time. Do we ever do inplace avsc changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well that is the point, protocol changes should be reviewed in isolation, prior to any code getting written 😁


jobs:
enforce-lines-changed:
runs-on: ubuntu-latest

steps:
- name: Checkout code
uses: actions/checkout@v3

- name: Get PR Metadata
id: pr_details
uses: actions-ecosystem/action-get-PR@v2
with:
token: ${{ secrets.GITHUB_TOKEN }}

- name: Check for Override Keyword
id: check_override
run: |
# Define the keyword for override
OVERRIDE_KEYWORD="override"

# Check PR title and body for the keyword
if echo "${{ steps.pr_details.outputs.title }}${{ steps.pr_details.outputs.body }}" | grep -iq "$OVERRIDE_KEYWORD"; then
echo "Override keyword found. Skipping validation."
echo "override=true" >> $GITHUB_OUTPUT
else
echo "override=false" >> $GITHUB_OUTPUT
fi
- name: Calculate Lines Changed Per File
id: lines_changed_per_file
run: |
# Define the maximum allowed lines changed per file
MAX_LINES=500

# Get the diff of the PR and process each file
EXCEEDED=false
while IFS=$'\t' read -r added removed file; do
# Skip test files
if [[ "${file,,}" == *test.java ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean test class files must end with the suffix Test? That does not seem right, since we don't follow this convention consistently enough. Can we instead rely on the full path needing to contain src/main in order to be included?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Let me update that to check for full path, though developer should include "test" somewhere in the class name.

echo "Skipping test file: $file"
continue
fi

# Calculate total lines changed for the file
TOTAL=$((added + removed))
echo "File: $file, Lines Changed: $TOTAL"

# Check if the total exceeds the max allowed
if [ "$TOTAL" -gt "$MAX_LINES" ]; then
echo "File $file exceeds the maximum allowed lines changed ($TOTAL > $MAX_LINES)"
EXCEEDED=true
fi
done < <(git diff --numstat origin/main | grep '\.java$')

# Fail if any file exceeded the limit
if [ "$EXCEEDED" = true ]; then
echo "Some files exceed the maximum allowed lines changed ($MAX_LINES)."
exit 1
fi

- name: Notify
if: failure()
run: |
echo "One or more files in the PR exceed the maximum allowed lines changed. Please review and adjust your changes to your files."