add only_changed input to run rubocop only against changed files #103

toy · 2024-05-02T18:08:30Z

Add an input to limit running rubocop only against files changed in the PR.

Had to switch to bash for process substitution and working with arrays.

Ignore if there are more than 100 changed files. Main reason is to protect from possibly hitting the command line length limit. In reality the command line length limit most probably (depends on OS) allows much higher number of files and if needed can be calculated or provided as one more input.

Fix ci workflow to fetch all commits for pr branch plus head commit of base branch to be able to find changed files.

~~Will conflict with #102, I will update whichever is not merged first~~

toy · 2024-05-23T16:49:13Z

@javierjulio May I ping you for input?

haya14busa · 2024-06-25T13:45:00Z

Can you add a test? #103

toy · 2024-06-25T14:31:38Z

@haya14busa Can you suggest how/what to test? I can think of either adding test branches and test workflows to check complete thing, or just unit testing script.sh similar to how it is done in test/rdjson_formatter/test.sh

thiemonipro · 2024-06-26T08:42:33Z

action.yml

@@ -70,6 +74,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 }}


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 github.event.pull_request.base.sha instead?

I tried out your branch in a github action workflow but the HEAD_REF commit seemed wrong and gave me the following error: fatal: Invalid revision range

After creating my own branch based on this but with the change to the HEAD_REF sha the action succeeded

I guess you need git fetch.

https://github.com/reviewdog/action-rubocop/pull/103/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR14

Do you mean using github.event.pull_request.head.sha for HEAD_REF?

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 actions/checkout with default depth: 1 is assumed.

Also github.sha is pointing at automatically created merge commit of base into head (docs), while github.event.pull_request.head.sha points at the PR branch head.

If we would have all commits available it would be simpler to do diff base...head (note three dots) where head can be both merge commit or the tip of the branch, but if we fetch only two tips of the branches, diff base...head is not possible, as not all needed commits are available.
Otherwise diff base..merge-head should give much smaller diff then diff base..head.

So most probable cause of fatal: Invalid revision range is because your workflow either doesn't fetch the tip of the base branch or fetches not the automatically created merge head (default using actions/checkout).

haya14busa · 2024-06-26T10:13:11Z

I think we can add a job (or workflow) with only_changed=true and add new test files under test dir.

We can at least check the behavior with this pr.
As you mentioned it, it would be cool if we can prepare a test branch too, but it's optional.

haya14busa · 2024-06-26T10:14:09Z

script.sh

@@ -85,9 +88,41 @@ else
  BUNDLE_EXEC="bundle exec "
 fi

+if [ "${INPUT_ONLY_CHANGED}" = "true" ]; then
+  echo '::group:: Getting changed files list'
+


How about running `git fetch --depth 1 origin "${BASE_REF}" here?

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.

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

haya14busa · 2024-06-26T10:16:46Z

Ideally, it would be great if we can use the existing solution like https://github.com/tj-actions/changed-files for getting changed files so that we can reuse the similar solution for other reviewdog actions.

But we use robocop specific command (rubocop --list-target-files ), so it's optional.

Switch to bash for process substitution and working with arrays. Ignore if there are more than 100. Protecting from possibly hitting the command line length limit, it can be much higher and can be calculated or also extracted as one more input if needed. The effect from limiting number of files processed by rubocop is also smaller. Fix ci workflow to fetch all commits for pr branch plus head commit of base branch to be able to find changed files. Co-authored-by: Oliver Günther <mail@oliverguenther.de>

v-kumar · 2024-07-12T13:31:01Z

Any update on this? Would love to see this merged.

toy · 2024-07-12T13:43:41Z

@v-kumar I did an unsuccessful attempt and afterwards came up with a working way to test this, but didn't yet have time. The idea is to test by running the script while mocking binaries by manipulating PATH

toy · 2024-07-12T20:25:49Z

@haya14busa I added tests of script.sh without using any frameworks

haya14busa · 2024-07-13T09:12:05Z

.github/workflows/ci.yml

+      - name: Check when there are too many relevant files
+        run: |
+          git checkout ${{ github.sha }}
+          touch a{00..100}.rb


Should we create 101 (more than 100) files here?

The range is inclusive, so 101 files are generated, I can change to {01..101} if it will be clearer

I accidentally tested that with exactly 100 it passes file names to rubocop: https://github.com/reviewdog/action-rubocop/actions/runs/9913233268/job/27389778004

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

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.

haya14busa

Thank you for your contribution! 💯

review-dog · 2024-07-14T12:51:25Z

Hi, @toy! We merged your PR to reviewdog! 🐶
Thank you for your contribution! ✨

We just invited you to join the @reviewdog organization on GitHub.
Accept the invite by visiting https://github.com/orgs/reviewdog/invitation.
By joining the team, you'll be a part of reviewdog community and can help the maintenance of reviewdog.

Thanks again!

github-actions · 2024-07-14T12:51:29Z

🚀 [bumpr] Bumped! New version:v2.17.0 Changes:v2.16.0...v2.17.0

kehra · 2024-08-21T02:17:39Z

script.sh

+  ${INPUT_RUBOCOP_FLAGS} \
+  --require ${GITHUB_ACTION_PATH}/rdjson_formatter/rdjson_formatter.rb \
+  --format RdjsonFormatter \
+  --fail-level error \


@toy
Did you want to intentionally change(set) fail-level?
I think it seems a bug.

@kehra It was intentional, due to turning on pipefail, which in turn is needed for pipes in getting the list of changed files.
Before this commit the exit code of rubocop … | reviewdog … was the exit code of reviewdog. But with pipefail and without changing fail-level, any rubocop violation would make the script have an error exit code, which will be confusing, as both violations will be reported and the action will fail.

@toy
Thank you for your response.
I think this change caused our CI to miss non-error level(e.g. warning) violations.
However, my colleague has made it possible to specify the fail-level in #120, so it is not a problem 😄

@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?

@toy
Hmm.
I might be wrong, so I'll investigate this again 💦
Sorry for bothering you.

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

Since PR reviewdog#103 introduced the default --fail-level flag, the severity level has become unchangeable due to this modification. Ideally, this flag (and others) should be overridable, but the RuboCop command prioritizes the flags that are specified last. This commit moves the user-specified flags to the end of the hard-coded flags, allowing them to be overridden. Signed-off-by: moznion <moznion@mail.moznion.net>

Since PR #103 introduced the default --fail-level flag, the severity level has become unchangeable due to this modification. Ideally, this flag (and others) should be overridable, but the RuboCop command prioritizes the flags that are specified last. This commit moves the user-specified flags to the end of the hard-coded flags, allowing them to be overridden. Signed-off-by: moznion <moznion@mail.moznion.net>

toy force-pushed the option-to-test-only-changed-files-reviewdog branch 4 times, most recently from 2aac704 to cd3cfda Compare May 8, 2024 16:10

toy mentioned this pull request May 13, 2024

Order inputs alphabetically #102

Merged

toy force-pushed the option-to-test-only-changed-files-reviewdog branch from cd3cfda to 998b37a Compare May 15, 2024 17:37

toy force-pushed the option-to-test-only-changed-files-reviewdog branch 3 times, most recently from 02f0ae4 to c83f927 Compare May 31, 2024 11:56

toy force-pushed the option-to-test-only-changed-files-reviewdog branch from c83f927 to 2719576 Compare June 25, 2024 12:10

thiemonipro reviewed Jun 26, 2024

View reviewed changes

thiemonipro mentioned this pull request Jun 26, 2024

Add flag to only execute rubocop on files changed in a PR #110

Closed

haya14busa reviewed Jun 26, 2024

View reviewed changes

toy force-pushed the option-to-test-only-changed-files-reviewdog branch from eb0eda0 to 5ea6c6e Compare June 27, 2024 17:40

toy and others added 3 commits June 28, 2024 14:07

automatically fetch the tip of the base branch if it is not available

50c80c5

don't hide the name of the CI job

e50a873

toy force-pushed the option-to-test-only-changed-files-reviewdog branch from 5ea6c6e to 705dbf7 Compare July 12, 2024 19:55

toy requested a review from haya14busa July 12, 2024 19:56

toy force-pushed the option-to-test-only-changed-files-reviewdog branch from 13a7ab4 to 4e840d4 Compare July 12, 2024 20:21

haya14busa reviewed Jul 13, 2024

View reviewed changes

toy force-pushed the option-to-test-only-changed-files-reviewdog branch from 4e840d4 to 2f1682e Compare July 14, 2024 10:52

test only_changed

baffa02

toy force-pushed the option-to-test-only-changed-files-reviewdog branch from 2f1682e to baffa02 Compare July 14, 2024 10:58

haya14busa approved these changes Jul 14, 2024

View reviewed changes

haya14busa added the bump:minor label Jul 14, 2024

haya14busa merged commit e44fb2c into reviewdog:master Jul 14, 2024
8 checks passed

toy deleted the option-to-test-only-changed-files-reviewdog branch July 14, 2024 20:23

kehra reviewed Aug 21, 2024

View reviewed changes

moznion mentioned this pull request Aug 21, 2024

Put ${INPUT_RUBOCOP_FLAGS} at the end of the other flags #120

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add only_changed input to run rubocop only against changed files #103

add only_changed input to run rubocop only against changed files #103

toy commented May 2, 2024 •

edited

Loading

toy commented May 23, 2024

haya14busa commented Jun 25, 2024

toy commented Jun 25, 2024

thiemonipro Jun 26, 2024 •

edited

Loading

haya14busa Jun 26, 2024

toy Jun 26, 2024

haya14busa commented Jun 26, 2024

haya14busa Jun 26, 2024

toy Jun 26, 2024

toy Jun 26, 2024

haya14busa commented Jun 26, 2024

v-kumar commented Jul 12, 2024

toy commented Jul 12, 2024 •

edited

Loading

toy commented Jul 12, 2024

haya14busa Jul 13, 2024

toy Jul 13, 2024

toy Jul 13, 2024

haya14busa Jul 14, 2024

toy Jul 14, 2024 •

edited

Loading

haya14busa left a comment

review-dog commented Jul 14, 2024

github-actions bot commented Jul 14, 2024

kehra Aug 21, 2024 •

edited

Loading

toy Aug 21, 2024

kehra Aug 21, 2024

toy Aug 21, 2024

kehra Aug 21, 2024

toy Aug 21, 2024

add only_changed input to run rubocop only against changed files #103

add only_changed input to run rubocop only against changed files #103

Conversation

toy commented May 2, 2024 • edited Loading

toy commented May 23, 2024

haya14busa commented Jun 25, 2024

toy commented Jun 25, 2024

thiemonipro Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haya14busa commented Jun 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haya14busa commented Jun 26, 2024

v-kumar commented Jul 12, 2024

toy commented Jul 12, 2024 • edited Loading

toy commented Jul 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toy Jul 14, 2024 • edited Loading

Choose a reason for hiding this comment

haya14busa left a comment

Choose a reason for hiding this comment

review-dog commented Jul 14, 2024

github-actions bot commented Jul 14, 2024

kehra Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toy commented May 2, 2024 •

edited

Loading

thiemonipro Jun 26, 2024 •

edited

Loading

toy commented Jul 12, 2024 •

edited

Loading

toy Jul 14, 2024 •

edited

Loading

kehra Aug 21, 2024 •

edited

Loading