packwerk: [Bug] Packwerk.yml `exclude` option does not work

Description packwerk does not exclude configured folders for exclusion.

To Reproduce

Demo rails app: https://github.com/evaldasg/packwerk-issue

Install npm package:

yarn add yaml-js

packwerk configured to exclude node_modules

exclude:
- "{bin,node_modules,script,tmp,vendor}/**/*"

packwerk validation fails:

➜  packwerk-issue git:(main) bin/packwerk validate
📦 Packwerk is running validation...

Validation failed ❗
Unknown keys in /packwerk-issue/node_modules/yaml-js/src/package.yml: ["name", "version", "description", "main", "repository", "devDependencies", "license"]
If you think a key should be included in your package.yml, please open an issue in https://github.com/Shopify/packwerk

Expected Behaviour If packwerk.yml configured with exclude option, it is expected that packwerk will skip those paths.

Version Information

Packwerk: [1.1.2]
Ruby [2.7.2]
Rails [6.0.3.5]

Additional Context Suspected code: https://github.com/Shopify/packwerk/blob/main/lib/packwerk/package_set.rb#L37

def package_paths(root_path, package_pathspec)
  bundle_path_match = Bundler.bundle_path.join("**").to_s

  glob_patterns = Array(package_pathspec).map do |pathspec|
    File.join(root_path, pathspec, PACKAGE_CONFIG_FILENAME)
  end

  Dir.glob(glob_patterns)
    .map { |path| Pathname.new(path).cleanpath }
    .reject { |path| path.realpath.fnmatch(bundle_path_match) }
end

This rejects only gems directory but does not take @configuration.exclude into account.

About this issue

Original URL
State: closed
Created 3 years ago
Comments: 15 (12 by maintainers)

Most upvoted comments

Fixed by @jordanstephens in https://github.com/Shopify/packwerk/pull/155 - Thank you Jordan!

exterm on Nov 24, 2021

Just tested it and running

      manifests.reject do |manifest_path|
        @configuration.exclude.any? do |exclude_pattern|
          File.fnmatch(File.expand_path(exclude_pattern), manifest_path, File::FNM_EXTGLOB)
        end
      end

Takes virtually no time on our largest app. Now, this app has only 67 packages (most packages are pretty large), but I still think it’s a good indication that there won’t be a noticeable performance impact from this.

I’d actually be curious if anyone reading this has an app with more packages, and if so how many!

Summarized - let’s explore a PR for this. It looks like integrating into PackageSet is the way to go, unless someone has a differing opinion.

exterm on Nov 11, 2021

# this always will differ
package_manifests("**/") - package_manifests(some_value_from_packwerk_yml)

That’s not true. The intention of the validation is that the package_paths option, which is an optional optimization to speed up analysis, is not set to a value that misses some packwerk packages.

However, I think we may be able to find a way to respect the ignore config that makes sense in this case.

exterm on Aug 27, 2021

ugh, OK! That seems like a problem.

exterm on Jul 12, 2021

@evaldasg Can you post the package.yml file that contains the keys?

The issue you are seeing doesn’t seem to be related to Packwerk reading excluded files. It sounds like you didn’t have your package.yml file set up properly, and the issue is caught by the validation check.

More specifically, packwerk seems to be picking up the keys [name", "version", "description", "main", "repository", "devDependencies", "license"] all of which are not valid keys for a package.yml file.

wildmaples on Mar 8, 2021