Extract privacy validations from ApplicationValidator #232

alexevanczuk · 2022-10-16T16:58:25Z

Commits

Move ApplicationValidator::Result to its own file
Move check_package_manifests_for_privacy to its own file
Move tests to own class
move more privacy things into the privacy specific checker
move tests to test/unit/validators/check_package_manifests_for_privacy_test.rb

What are you trying to accomplish?

This is one step in the direction of #219
To extract privacy checking, one thing we need to extract is validation conceptually coupled to privacy.
In this PR, I pull ApplicationValidator concerns related to privacy into their own class.

What approach did you choose and why?

I made it a class name namespaced under ApplicationValidator, for now, to make it as easy as possible to change references to defined constants
I used a method name "call" for the privacy validation. After extracting the remaining validations, this can be maintained via a sorbet interface that ensures a single API for validation. This would allow users to plugin their own validation that runs automatically with bin/packwerk validate. This is necessary so folks can easily continue to use privacy validation as expected without needing to change what APIs they call.
When there was a function in ApplicationValidator that no one else used, I made it a private function of the privacy validation.
When there was a function that many validations used, I extracted it to a shared, but explicitly private constant called Private. Later, when the rest of the validations are in their own class and we see what functions they all depend on, we can see what APIs make sense to expose to client-injected validations.
Rather than have each validation maintain its own instance state, I decided to pass in necessary dependencies, such as configuration, through to the privacy validation. This way it can be easy to see what input parameters are necessary for the hypothetical validation interface.
I moved tests into their own class in a later commit so its easy to see what changed about tests (nothing besides references to the API used to invoke the validation).

Note: The plan here is to extract the rest of the validations into ApplicationValidator so each validation is in its own class. This doesn't mean each validation would be extracted from the codebase or available as its own indepenent API (we should mark them with private_constant).

What should reviewers focus on?

Does this make sense as a plan to extract privacy while moving towards allowing client-injected validations?

Type of Change

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

Checklist

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

alexevanczuk · 2022-10-19T16:22:11Z

lib/packwerk/application_validator.rb

        hash = YAML.load_file(f)
        next unless hash

-        known_keys = ["enforce_privacy", "enforce_dependencies", "public_path", "dependencies", "metadata"]


We won't want the main validator knowing anything about enforce_privacy, so this can become a plugin concern.

I was thinking that a plugin can implement one or both interfaces – a checker interface or a validator interface (or just a single Plugin interface which is both of these). The validator would load those and allow top-level keys that plugins can specify.

alexevanczuk · 2022-10-19T16:22:39Z

lib/packwerk/application_validator/check_package_manifests_for_privacy.rb

+
+module Packwerk
+  class ApplicationValidator
+    class CheckPackageManifestsForPrivacy


This is quite a lot of validation logic that we'll be able to extract out of packwerk!

lib/packwerk/application_validator/private.rb

test/unit/validators/check_package_manifests_for_privacy_test.rb

gmcgibbon · 2022-10-26T05:13:18Z

lib/packwerk/application_validator/check_package_manifests_for_privacy.rb

+
+module Packwerk
+  class ApplicationValidator
+    class CheckPackageManifestsForPrivacy


Why is this a class if we never instantiate it?

This is going to need to be merged in with the privacy stuff soon and implement the Packwerk::PluginInterface. Only a class can implement a sorbet interface. This is also all code that will soon be deleted from packwerk and moved out.

Similar to #233 (comment) it is difficult to see the end result when all I can see in this change is just moving functions around. Have we thought thought the extraction end-to-end already, or is the end result still a work in progress?

Hey -- let me know if the chat we had over slack helped resolve this or if there are any specific changes you'd like me to make to this PR.

I would change it at that time. Right now there's no need to be a class.

Sounds good – making it a module now.

lib/packwerk/application_validator/check_package_manifests_for_privacy.rb

alexevanczuk · 2022-10-26T14:22:10Z

@gmcgibbon Responded to all your feedback and pushed up changes!

alexevanczuk · 2022-10-31T13:41:05Z

@gmcgibbon Could you take another look?

gmcgibbon · 2022-10-31T17:36:03Z

lib/packwerk/application_validator/check_package_manifests_for_privacy.rb

+
+module Packwerk
+  class ApplicationValidator
+    class CheckPackageManifestsForPrivacy


I would change it at that time. Right now there's no need to be a class.

gmcgibbon · 2022-10-31T17:46:04Z

lib/packwerk/application_validator/helpers.rb

+
+module Packwerk
+  class ApplicationValidator
+    module Helpers


I'm not 100% sold on placing helpers in one big module. I think it would be more useful to define a subclass that extensions like the privacy checker can extend from. However, this is good enough for now as long as it stays private.

Yep I agree with ya... I think once we have a stronger sense of which parts of Helpers should be supported public API for checkers/validators we can pull that into some public subclass 👍

mclark

This is a nice improvement 👍

mclark · 2022-10-31T17:44:28Z

lib/packwerk/application_validator.rb

@@ -11,6 +11,13 @@ module Packwerk
  class ApplicationValidator
    extend T::Sig

+    # This is a temporary API as we migrate validators to their own files.
+    # Later, we can expose an API to get package sets to pass into validators when testing
+    # This API would likely just be `PackageSet.load_all_from(configruation)`, but we might want to clean


Suggested change

# This API would likely just be `PackageSet.load_all_from(configruation)`, but we might want to clean

# This API would likely just be `PackageSet.load_all_from(configuration)`, but we might want to clean

mclark · 2022-10-31T17:48:31Z

lib/packwerk/application_validator/helpers.rb

+        end
+
+        sig { params(configuration: Configuration).returns(T.any(T::Array[String], String)) }
+        def package_glob(configuration)


I wonder if this method would be better hung off Configuration instead as that's a non-null argument anyway? 🤔

mclark · 2022-10-31T17:52:10Z

lib/packwerk/application_validator/helpers.rb

+          params(configuration: Configuration,
+            glob_pattern: T.nilable(T.any(T::Array[String], String))).returns(T::Array[String])
+        end
+        def package_manifests(configuration, glob_pattern = nil)


This also feels like a method that might better belong to Configuration too.

mclark · 2022-10-31T18:06:26Z

lib/packwerk/application_validator/helpers.rb

+        extend T::Sig
+
+        sig { params(configuration: Configuration, setting: T.untyped).returns(T.untyped) }
+        def package_manifests_settings_for(configuration, setting)


Yeah I'm feeling like we have a missing abstraction here that the name "Helpers" is sort of hinting at. I'm not sure yet what it might be but I think something hanging off Configuration is feeling likely. Configuration#package_manifests(glob_pattern = nil) which returns T::Array[PackageManifest] maybe. Mostly just typing and thinking now, I think putting stuff in Helpers for now should be fine.

alexevanczuk requested a review from a team as a code owner October 16, 2022 16:58

alexevanczuk force-pushed the ae-break-up-application-validator branch from 7c40fd8 to 17b5683 Compare October 16, 2022 16:58

alexevanczuk self-assigned this Oct 16, 2022

alexevanczuk force-pushed the ae-break-up-application-validator branch from 17b5683 to 7a0b10d Compare October 16, 2022 17:02

alexevanczuk mentioned this pull request Oct 17, 2022

Colocate privacy API on Packwerk::Package #233

Merged

6 tasks

alexevanczuk force-pushed the ae-break-up-application-validator branch from 7a0b10d to df5ccf2 Compare October 19, 2022 16:20

alexevanczuk commented Oct 19, 2022

View reviewed changes

alexevanczuk force-pushed the ae-break-up-application-validator branch from e994216 to e41fb6a Compare October 19, 2022 20:56

gmcgibbon reviewed Oct 26, 2022

View reviewed changes

alexevanczuk requested a review from gmcgibbon October 26, 2022 14:21

alexevanczuk force-pushed the ae-break-up-application-validator branch 2 times, most recently from 6ac255b to aa142d9 Compare October 28, 2022 20:16

alexevanczuk force-pushed the ae-break-up-application-validator branch from aa142d9 to 2bf98dd Compare October 31, 2022 17:39

gmcgibbon approved these changes Oct 31, 2022

View reviewed changes

Extract privacy validations from ApplicationValidator

5bbedf1

alexevanczuk force-pushed the ae-break-up-application-validator branch from fe5f234 to 5bbedf1 Compare October 31, 2022 17:51

alexevanczuk merged commit 589fe52 into main Oct 31, 2022

alexevanczuk deleted the ae-break-up-application-validator branch October 31, 2022 18:03

mclark reviewed Oct 31, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract privacy validations from ApplicationValidator #232

Extract privacy validations from ApplicationValidator #232

alexevanczuk commented Oct 16, 2022 •

edited

Loading

alexevanczuk Oct 19, 2022

alexevanczuk Oct 19, 2022

gmcgibbon Oct 26, 2022

alexevanczuk Oct 26, 2022

gmcgibbon Oct 28, 2022

alexevanczuk Oct 31, 2022

gmcgibbon Oct 31, 2022

alexevanczuk Oct 31, 2022

alexevanczuk commented Oct 26, 2022

alexevanczuk commented Oct 31, 2022

gmcgibbon Oct 31, 2022

gmcgibbon Oct 31, 2022

alexevanczuk Oct 31, 2022

mclark left a comment

mclark Oct 31, 2022

mclark Oct 31, 2022

mclark Oct 31, 2022

mclark Oct 31, 2022

	# This API would likely just be `PackageSet.load_all_from(configruation)`, but we might want to clean
	# This API would likely just be `PackageSet.load_all_from(configuration)`, but we might want to clean

Extract privacy validations from ApplicationValidator #232

Extract privacy validations from ApplicationValidator #232

Conversation

alexevanczuk commented Oct 16, 2022 • edited Loading

Commits

What are you trying to accomplish?

What approach did you choose and why?

What should reviewers focus on?

Type of Change

Checklist

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexevanczuk commented Oct 26, 2022

alexevanczuk commented Oct 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mclark left a 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

alexevanczuk commented Oct 16, 2022 •

edited

Loading