-
Notifications
You must be signed in to change notification settings - Fork 75
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
add only_changed input to run rubocop only against changed files #103
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,12 @@ inputs: | |
level: | ||
description: 'Report level for reviewdog [info,warning,error]' | ||
default: 'error' | ||
only_changed: | ||
description: | | ||
Run Rubocop only on changed (and added) files, for speedup [`true`, `false`]. | ||
Will fetch the tip of the base branch with depth 1 from remote origin if it is not available. | ||
If you use different remote name or customize the checkout otherwise, make the tip of the base branch available before this action. | ||
default: 'false' | ||
reporter: | ||
description: | | ||
Reporter of reviewdog command [github-pr-check,github-check,github-pr-review]. | ||
|
@@ -61,6 +67,7 @@ runs: | |
INPUT_FILTER_MODE: ${{ inputs.filter_mode }} | ||
INPUT_GITHUB_TOKEN: ${{ inputs.github_token }} | ||
INPUT_LEVEL: ${{ inputs.level }} | ||
INPUT_ONLY_CHANGED: ${{ inputs.only_changed }} | ||
INPUT_REPORTER: ${{ inputs.reporter }} | ||
INPUT_REVIEWDOG_FLAGS: ${{ inputs.reviewdog_flags }} | ||
INPUT_RUBOCOP_EXTENSIONS: ${{ inputs.rubocop_extensions }} | ||
|
@@ -70,6 +77,8 @@ runs: | |
INPUT_TOOL_NAME: ${{ inputs.tool_name }} | ||
INPUT_USE_BUNDLER: ${{ inputs.use_bundler }} | ||
INPUT_WORKDIR: ${{ inputs.workdir }} | ||
BASE_REF: ${{ github.event.pull_request.base.sha }} | ||
HEAD_REF: ${{ github.sha }} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. First off all thanks for this PR, it seems like a really useful addition that could speed up our github action by a lot as using rubocop takes about 5 minutes currently to run over the whole repo. Should this look at I tried out your branch in a github action workflow but the HEAD_REF commit seemed wrong and gave me the following error: After creating my own branch based on this but with the change to the HEAD_REF sha the action succeeded There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean using The idea of this change is to give to rubocop only files introduced in the PR branch, but we don't want to also give to rubocop files that changed on the base branch especially if it diverged a lot. This could be resolved in different ways, but in current implementation the idea is to also fetch bare minimum to not slowdown by fetching, so usage of Also If we would have all commits available it would be simpler to do So most probable cause of |
||
branding: | ||
icon: 'check-circle' | ||
color: 'red' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,7 @@ | ||
#!/bin/sh -e | ||
#!/usr/bin/env bash | ||
|
||
set -e | ||
set -o pipefail | ||
|
||
cd "${GITHUB_WORKSPACE}/${INPUT_WORKDIR}" || exit | ||
export REVIEWDOG_GITHUB_API_TOKEN="${INPUT_GITHUB_TOKEN}" | ||
|
@@ -85,9 +88,46 @@ else | |
BUNDLE_EXEC="bundle exec " | ||
fi | ||
|
||
if [ "${INPUT_ONLY_CHANGED}" = "true" ]; then | ||
echo '::group:: Getting changed files list' | ||
|
||
# check if commit is present in repository, otherwise fetch it | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about running `git fetch --depth 1 origin "${BASE_REF}" here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like a good idea, compared to adding a step to do it manually. Only concern is if remote is named differently or if checkout is very custom, so the command breaks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doing it after checking commit availability should be the good way to make it simple by default and solvable if default doesn't work: eb0eda0 |
||
if ! git cat-file -e "${BASE_REF}"; then | ||
git fetch --depth 1 origin "${BASE_REF}" | ||
fi | ||
|
||
# get intersection of changed files (excluding deleted) with target files for | ||
# rubocop as an array | ||
# shellcheck disable=SC2086 | ||
readarray -t CHANGED_FILES < <( | ||
comm -12 \ | ||
<(git diff --diff-filter=d --name-only "${BASE_REF}..${HEAD_REF}" | sort || kill $$) \ | ||
<(${BUNDLE_EXEC}rubocop --list-target-files | sort || kill $$) | ||
) | ||
|
||
if (( ${#CHANGED_FILES[@]} == 0 )); then | ||
echo "No relevant files for rubocop, skipping" | ||
exit 0 | ||
fi | ||
|
||
printf '%s\n' "${CHANGED_FILES[@]}" | ||
|
||
if (( ${#CHANGED_FILES[@]} > 100 )); then | ||
echo "More than 100 changed files (${#CHANGED_FILES[@]}), running rubocop on all files" | ||
unset CHANGED_FILES | ||
fi | ||
|
||
echo '::endgroup::' | ||
fi | ||
|
||
echo '::group:: Running rubocop with reviewdog 🐶 ...' | ||
# shellcheck disable=SC2086 | ||
${BUNDLE_EXEC}rubocop ${INPUT_RUBOCOP_FLAGS} --require ${GITHUB_ACTION_PATH}/rdjson_formatter/rdjson_formatter.rb --format RdjsonFormatter \ | ||
${BUNDLE_EXEC}rubocop \ | ||
${INPUT_RUBOCOP_FLAGS} \ | ||
--require ${GITHUB_ACTION_PATH}/rdjson_formatter/rdjson_formatter.rb \ | ||
--format RdjsonFormatter \ | ||
--fail-level error \ | ||
"${CHANGED_FILES[@]}" \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @toy There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kehra It was intentional, due to turning on pipefail, which in turn is needed for pipes in getting the list of changed files. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kehra #120 looks good, it allows to "shoot in the foot" by changing formatter, but I think it is better to restrict less. I'm still wondering about your CI - any violations of any level will create either annotation or comment to PR, so the rubocop outcome will be failing, but not the rubocop action itself, am I missing something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @toy There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No worries, I'm just trying to understand if there is some case that allows action to not report violations. Please reply with your findings |
||
| reviewdog -f=rdjson \ | ||
-name="${INPUT_TOOL_NAME}" \ | ||
-reporter="${INPUT_REPORTER}" \ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
puts "Hello, " + "world!" | ||
Check warning on line 1 in test/only_changed/few_relevant/files/a.rb GitHub Actions / test-skip-install-and-use-bundler
Check warning on line 1 in test/only_changed/few_relevant/files/a.rb GitHub Actions / test-skip-install-and-use-bundler
Check warning on line 1 in test/only_changed/few_relevant/files/a.rb GitHub Actions / test-skip-install-and-use-bundler
Check warning on line 1 in test/only_changed/few_relevant/files/a.rb GitHub Actions / test-skip-install-and-use-bundler
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
puts "Hello, " + "world!" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
#!/usr/bin/env ruby | ||
# frozen_string_literal: true | ||
|
||
case ARGV | ||
when %w[ | ||
--list-target-files | ||
] | ||
puts Dir['**/*.rb'] | ||
when %W[ | ||
--require #{ENV['GITHUB_ACTION_PATH']}/rdjson_formatter/rdjson_formatter.rb | ||
--format RdjsonFormatter | ||
--fail-level error | ||
a.rb | ||
] | ||
puts 'Mock message for reviewdog' | ||
else | ||
abort "rubocop mock called with unexpected arguments:\n#{ARGV.join("\n")}" | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
puts "Hello, " + "world!" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
#!/usr/bin/env ruby | ||
# frozen_string_literal: true | ||
|
||
case ARGV | ||
when %w[ | ||
--list-target-files | ||
] | ||
puts Dir['**/*.rb'] | ||
else | ||
abort "rubocop mock called with unexpected arguments:\n#{ARGV.join("\n")}" | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
#!/bin/bash | ||
|
||
if [ "$1" = "exec" ]; then | ||
shift | ||
eval "$@" | ||
else | ||
echo "Only 'exec' command is supported" | ||
exit 1 | ||
fi |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
#!/bin/bash | ||
|
||
arguments="$*" | ||
|
||
if [ "$arguments" != "-sfL https://raw.githubusercontent.com/reviewdog/reviewdog/master/install.sh" ]; then | ||
echo "curl mock got unexpected arguments: $arguments" | ||
exit 1 | ||
fi |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
#!/bin/bash | ||
|
||
touch test/only_changed/reviewdog-was-called | ||
|
||
read -r input | ||
|
||
if [ "$input" != "Mock message for reviewdog" ]; then | ||
echo "reviewdog mock got unexpected input: $input" | ||
exit 1 | ||
fi |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
#!/usr/bin/env ruby | ||
# frozen_string_literal: true | ||
|
||
case ARGV | ||
when %w[ | ||
--list-target-files | ||
] | ||
puts Dir['**/*.rb'] | ||
when %W[ | ||
--require #{ENV['GITHUB_ACTION_PATH']}/rdjson_formatter/rdjson_formatter.rb | ||
--format RdjsonFormatter | ||
--fail-level error | ||
] | ||
puts 'Mock message for reviewdog' | ||
else | ||
abort "rubocop mock called with unexpected arguments:\n#{ARGV.join("\n")}" | ||
end |
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.
Should we create 101 (more than 100) files here?
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.
The range is inclusive, so 101 files are generated, I can change to
{01..101}
if it will be clearerThere 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.
I accidentally tested that with exactly 100 it passes file names to rubocop: https://github.com/reviewdog/action-rubocop/actions/runs/9913233268/job/27389778004
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.
ok.
I'm a bit confused, but why does the test result shows " No relevant files for rubocop, skipping"?
https://github.com/reviewdog/action-rubocop/actions/runs/9913690348/job/27391225472
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.
Great that you spotted it, I didn't notice that change from ruby to bash script for rubocop mock was producing files prefixed with
./
. I reverted to ruby script for simplicity and hardened the test by explicitly validating that reviewdog mock was or was not called.