Skip to content

Fix RuboCop excludes not respected in the updater CI#5609

Merged
deivid-rodriguez merged 1 commit intomainfrom
deivid-rodriguez/fix-rubocop-excludes-not-respected
Jan 13, 2023
Merged

Fix RuboCop excludes not respected in the updater CI#5609
deivid-rodriguez merged 1 commit intomainfrom
deivid-rodriguez/fix-rubocop-excludes-not-respected

Conversation

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

@deivid-rodriguez deivid-rodriguez commented Sep 1, 2022

In the Dockerfile.updater, the layout of rubocop configurations is the following:

~/.rubocop.yml
~/updater/.rubocop.yml

Note that this slightly differs from the setup of other "components" in the Dockerfile.ci. For example:

~/dependabot-core/.rubocop.yml
~/dependabot-core/bundler/.rubocop.yml

This subtle difference makes running the script/lint script in the updater's CI fail, so we had to workaround that by passing explicit folders to RuboCop (docker_bundle_exec rubocop lib/ spec/)

The reason is that in the updater CI, exclusions in the main RuboCop config are not correctly interpreted, because they are resolved using the current directory as a base, not the directory of the configuration file itself.

So the following exclusion

---
AllCops:
  DisplayCopNames: true
  Exclude:
  - "*/vendor/**/*"

is interpreted as ~/updater/*/vendor/**/*, so the stuff installed at ~/updater/vendor/**/* is not properly ignored.

The reason for this is that a ~/.rubocop.yml configuration (located at the HOME folder) has special behavior with respect to exclusions. Normally, this file is shared as a base configuration to be shared among all your applications, so the special behavior of making exclude patterns be based on the PWD instead of on the path to the configuration itself makes sense to me. BUT, it breaks our particular case.

For the curious, this is where RuboCop special cases this:

https://github.com/rubocop/rubocop/blob/a643eaaa00dd65ba5aacff98b027e5b5a40c1561/lib/rubocop/config.rb#L219

The solution is to move the common configuration to live in the omnibus folder instead. This way, this special behavior does not affect us.

There were other alternatives like:

  • Keep the current workaround, but RuboCop also behaves specially when given explicit folders, so I think better to normalize things to avoid surprises.

  • Relax the current Exclude patterns to catch everything. That could work, but we leave the ~/.rubocop.yml file in place, and I believe the special behavior of this file will be likely to bite us again.

@deivid-rodriguez deivid-rodriguez requested review from a team September 1, 2022 16:59
@deivid-rodriguez deivid-rodriguez requested a review from a team as a code owner September 1, 2022 16:59
@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/fix-rubocop-excludes-not-respected branch from 0138b87 to 037c10a Compare September 1, 2022 17:16
@deivid-rodriguez
Copy link
Copy Markdown
Contributor Author

deivid-rodriguez commented Sep 1, 2022

I added 22ac37a, just to keep the same result when running rubocop from the repository root, but I don't think it's needed. Running rubocop from the repo root fails anyways.

@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/fix-rubocop-excludes-not-respected branch from 22ac37a to 7d622be Compare September 2, 2022 12:43
@deivid-rodriguez
Copy link
Copy Markdown
Contributor Author

To "proof" how this PR fixes things, on main this is the result of linting updater:

Inspecting 32 files
................................

32 files inspected, no offenses detected

And that list of files (you can pass -L to RuboCop to see it) includes, for example, files in updater/spec/fixtures/ which should be excluded.

With this PR the result is

Inspecting 28 files
............................

28 files inspected, no offenses detected

And the underlying list of inspected files is what it should be.

@deivid-rodriguez
Copy link
Copy Markdown
Contributor Author

This is still an issue, but I think the better solution would be to normalize the directory structure of the new merged repo. So I'll close this in favor of #5625.

@deivid-rodriguez deivid-rodriguez deleted the deivid-rodriguez/fix-rubocop-excludes-not-respected branch September 15, 2022 18:40
@deivid-rodriguez deivid-rodriguez restored the deivid-rodriguez/fix-rubocop-excludes-not-respected branch January 12, 2023 21:49
@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/fix-rubocop-excludes-not-respected branch from 7d622be to c745301 Compare January 12, 2023 22:02
Copy link
Copy Markdown
Member

@jakecoffman jakecoffman left a comment

Choose a reason for hiding this comment

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

Works for me!

@deivid-rodriguez
Copy link
Copy Markdown
Contributor Author

Still not enough to be able to kill Dockerfile.ci apparently. But should fix the lint issues! I can remove the commit I cherry-picked to remove Dockerfile.ci and leave that to your PR, one hurdle at a time!

In the `Dockerfile.updater`, the layout of rubocop configurations is
the following:

```
~/.rubocop.yml
~/updater/.rubocop.yml
```

Note that this slightly differs from the setup of other "components" in
the `Dockerfile.ci`. For example:

```
~/dependabot-core/.rubocop.yml
~/dependabot-core/bundler/.rubocop.yml
```

This subtle difference makes running the `script/lint` script in the
updater's CI fail, so we had to workaround that by passing explicit
folders to RuboCop (`docker_bundle_exec rubocop lib/ spec/`)

The reason is that in the updater CI, exclusions in the main RuboCop
config are not correctly interpreted, because they are resolved using
the current directory as a base, not the directory of the configuration
file itself.

So the following exclusion

```yaml
---
AllCops:
  DisplayCopNames: true
  Exclude:
  - "*/vendor/**/*"
```

is interpreted as `~/updater/*/vendor/**/*`, so the stuff installed at
`~/updater/vendor/**/*` is not properly ignored.

The reason for this is that a `~/.rubocop.yml` configuration (located at
the HOME folder) has special behavior with respect to exclusions.
Normally, this file is shared as a base configuration to be shared among
all your applications, so the special behavior of making exclude
patterns be based on the PWD instead of on the path to the configuration
itself makes sense to me. BUT, it breaks our particular case.

For the curious, this is where RuboCop special cases this:

https://github.com/rubocop/rubocop/blob/a643eaaa00dd65ba5aacff98b027e5b5a40c1561/lib/rubocop/config.rb#L211-L224

The solution is to move the common configuration to live in the omnibus
folder instead. This way, this special behavior does not affect us.

There were other alternatives like:

* Keep the current workaround, but RuboCop also behaves specially when
  given explicit folders, so I think better to normalize things to avoid
  surprises.

* Relax the current Exclude patterns to catch everything. That could
  work, but we leave the `~/.rubocop.yml` file in place, but I believe
  the special behavior of this file will be likely to bite us again.
@jakecoffman
Copy link
Copy Markdown
Member

Makes sense! 👍 Baby steps 👣

@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/fix-rubocop-excludes-not-respected branch from c745301 to 0a134d1 Compare January 13, 2023 14:17
@deivid-rodriguez deivid-rodriguez merged commit 88960b9 into main Jan 13, 2023
@deivid-rodriguez deivid-rodriguez deleted the deivid-rodriguez/fix-rubocop-excludes-not-respected branch January 13, 2023 16:19
@jeffwidman
Copy link
Copy Markdown
Member

Oh man, this was turtles all the way down with the rubocop special casing... thanks for digging into this Deivid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants