Add missing .realpath checks that was causing validation error #65

wildmaples · 2020-11-10T21:49:53Z

What are you trying to accomplish?

There are two bugs in v1.0.1 that results in packwerk validate failure because the paths were not real paths as it was recently removed. Because there was no test to catch it, this ended up in a release.

What approach did you choose and why?

There are two fixes in this PR. I have linked the appropriate PRs / line change that caused the bug.

What should reviewers focus on?

We should get this change out ASAP.

You can help by confirming this change with the steps below:

checkout a master branch of Core with a local version of Packwerk v1.0.1 linked
run bin/spring packwerk validate - this will fail
checkout this branch and run validate again - this should pass

Type of Change

Bugfix
New feature
Non-breaking change (a change that doesn't alter functionality - i.e., code refactor, configs, etc.)

Additional Release Notes

Breaking change (fix or feature that would cause existing functionality to change)

Include any notes here to include in the release description. For example, if you selected "breaking change" above, leave notes on how users can transition to this version.

If no additional notes are necessary, delete this section or leave it unchanged.

Checklist

I have updated the documentation accordingly.
I have added tests to cover my changes.
It is safe to rollback this change.

wildmaples · 2020-11-11T14:44:15Z

lib/packwerk/configuration.rb

        path = Pathname.new(path_string)

-        next unless path.exist?
+        # ignore paths outside of the Rails root
+        next unless path.exist? && path.realpath.fnmatch(Rails.root.join("**").to_s)


Previously we did the check to see if the path exist and if the realpath version matches a path inside the rails root. The latter part of the check was removed in this PR: https://github.com/Shopify/packwerk/pull/21/files#diff-e43184d1a960a3c3821da21b3eb72c83379c2dcc996f6bcfac9aadd6898cd374L64

Nitpick: it feels a bit clearer to me if these two conditionals exist on their own lines:

next unless path.exist? next unless path.realpath.fnmatch(Rails.root.join("**").to_s)

path.exist? is a precondition for the rest of the loop, not just for your new Rails path condition.

Can we test this new condition in configuration_test? Maybe by appending an external path to Rails.application.config.autoload_paths beforehand? Don't forget to restore the paths to their old value.

Can we test this new condition in configuration_test? Maybe by appending an external path to Rails.application.config.autoload_paths beforehand? Don't forget to restore the paths to their old value.

Tested in #68 where this logic was moved into its own class.

wildmaples · 2020-11-11T14:44:39Z

lib/packwerk/application_validator.rb

@@ -329,7 +329,8 @@ def package_glob
    end

    def package_manifests(glob_pattern)
-      PackageSet.package_paths(@configuration.root_path, glob_pattern).map(&:to_s)
+      PackageSet.package_paths(@configuration.root_path, glob_pattern)
+        .map { |f| File.realpath(f) }


The .realpath was removed in this PR: https://github.com/Shopify/packwerk/pull/38/files#diff-5ca9dcd54fd0b9eeef94e00ba6adc7822dc83e5dd2bfb9e8d96fd3c356de8b8aL328

I verified that File.realpath(Pathname) returns a string, so there's no need to add to_s like in the old code.

[9] pry(main)> File.realpath(Pathname.new(".")) => "/Users/rochlefebvre/src/github.com/Shopify/parsnip"

👍

\nit I'm thinking out loud here, but could it make sense to add a method to the PackageSet that returns all the realpaths for the given packages instead of ApplicationValidator being aware of retrieving that information? 🤔

exterm

If we can add a test, we can merge this. Thank you, Maple!

wildmaples · 2020-11-11T19:44:15Z

If we can add a test, we can merge this. Thank you, Maple!

I know this is not optimal, but I tried testing and it is hard. Two changes in this PR:

For the validator: we have an existing issue for ApplicationValidator unit tests and I believe @rochlefebvre is working on it.
The change within Configuration#all_application_autoload_paths is also quite hard to test without a huge refactor. We could move the Rails logic out of the method/class so we can make it easier to test but unfortunately that is without a larger refactor right now.

I don't want to avoid testing. Can you see any easy test suite I can add to account for either these two cases? Otherwise I vote to ship this change/release and work on the test in another PR.

exterm · 2020-11-11T20:52:18Z

The way I see it, all_application_autoload_paths does three things:

get all autoload paths out of rails
filter the paths to just the ones that are interesting to us
warn if no paths remain

The part that broke was 2.. If you can extract these three steps into separate methods, you should be able to easily test just step 2. It would just be a method that takes a list of paths as well as an application root path and a bundle path, and returns a list of paths.

…aths`

wildmaples · 2020-11-16T17:53:56Z

lib/packwerk/application_load_paths.rb

-        paths.map { |path| path.relative_path_from(rails_root).to_s }
+        paths
+          .map { |path| path.relative_path_from(rails_root).to_s }
+          .uniq


We need this .uniq because #filter_relevant_paths may result in some added load paths. This was causing a failure when I tophatted on Core.

The .uniq was removed from ApplicationValidator here, but I figure it is best to add it in this class so we can also test for it.

tomstuart

The code change looks correct to me. I tophatted it on core and it works. Thank you!

tonymarklove-babylon · 2020-12-01T15:29:42Z

Apologies for the breakage, everyone! I was just looking at the latest version and spotted that I managed to break an entire release. 🤦 It worked on our codebase!

wildmaples force-pushed the fix-realpath branch from 78b3786 to f197497 Compare November 11, 2020 14:37

wildmaples changed the title ~~Add .realpath back to #package_manifests~~ Add missing .realpath checks that was causing validation error Nov 11, 2020

wildmaples force-pushed the fix-realpath branch from f197497 to cb28507 Compare November 11, 2020 14:42

wildmaples commented Nov 11, 2020

View reviewed changes

wildmaples force-pushed the fix-realpath branch from cb28507 to 320a2c2 Compare November 11, 2020 14:48

wildmaples marked this pull request as ready for review November 11, 2020 14:51

wildmaples requested a review from a team as a code owner November 11, 2020 14:51

exterm approved these changes Nov 11, 2020

View reviewed changes

wildmaples mentioned this pull request Nov 12, 2020

Make load path extraction logic testable #68

Merged

7 tasks

rochlefebvre approved these changes Nov 13, 2020

View reviewed changes

wildmaples added 2 commits November 16, 2020 10:35

Add .realpath back to #package_manifests

1f6002d

Add space for readability

94598e1

wildmaples force-pushed the fix-realpath branch from 0e59851 to 94598e1 Compare November 16, 2020 15:42

Return unique load paths for `ApplicationLoadPaths#extract_relevant_p…

9fa4a28

…aths`

wildmaples commented Nov 16, 2020

View reviewed changes

doodzik approved these changes Nov 16, 2020

View reviewed changes

tomstuart approved these changes Nov 16, 2020

View reviewed changes

wildmaples merged commit 549ee51 into main Nov 16, 2020

wildmaples deleted the fix-realpath branch November 16, 2020 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing .realpath checks that was causing validation error #65

Add missing .realpath checks that was causing validation error #65

wildmaples commented Nov 10, 2020 •

edited

wildmaples Nov 11, 2020 •

edited

rochlefebvre Nov 11, 2020

rochlefebvre Nov 11, 2020

wildmaples Nov 16, 2020

wildmaples Nov 11, 2020 •

edited

rochlefebvre Nov 11, 2020

doodzik Nov 16, 2020

exterm left a comment

wildmaples commented Nov 11, 2020

exterm commented Nov 11, 2020

wildmaples Nov 16, 2020

wildmaples Nov 16, 2020 •

edited

tomstuart left a comment

tonymarklove-babylon commented Dec 1, 2020

Add missing .realpath checks that was causing validation error #65

Add missing .realpath checks that was causing validation error #65

Conversation

wildmaples commented Nov 10, 2020 • edited

What are you trying to accomplish?

What approach did you choose and why?

What should reviewers focus on?

Type of Change

Additional Release Notes

Checklist

wildmaples Nov 11, 2020 • edited

Choose a reason for hiding this comment

rochlefebvre Nov 11, 2020

Choose a reason for hiding this comment

rochlefebvre Nov 11, 2020

Choose a reason for hiding this comment

wildmaples Nov 16, 2020

Choose a reason for hiding this comment

wildmaples Nov 11, 2020 • edited

Choose a reason for hiding this comment

rochlefebvre Nov 11, 2020

Choose a reason for hiding this comment

doodzik Nov 16, 2020

Choose a reason for hiding this comment

exterm left a comment

Choose a reason for hiding this comment

wildmaples commented Nov 11, 2020

exterm commented Nov 11, 2020

wildmaples Nov 16, 2020

Choose a reason for hiding this comment

wildmaples Nov 16, 2020 • edited

Choose a reason for hiding this comment

tomstuart left a comment

Choose a reason for hiding this comment

tonymarklove-babylon commented Dec 1, 2020

wildmaples commented Nov 10, 2020 •

edited

wildmaples Nov 11, 2020 •

edited

wildmaples Nov 11, 2020 •

edited

wildmaples Nov 16, 2020 •

edited