Pull privacy concerns out of packwerk #247

alexevanczuk · 2022-10-31T20:36:35Z

Do not merge yet

I think sequencing should look something like this:

Release 2.3.0, with a note about deprecating deprecated_references.yml and bin/packwerk update-deprecations in favor of package_todo.yml and bin/packwerk update-todo, and a migration hint:

Dir['**/deprecated_references.yml'].each{|f| File.rename(f, f.gsub('deprecated_references', 'package_todo')) }

Land this
Land Remove deprecated things in advance of major version release #248
Release 3.0.0

What are you trying to accomplish?

This PR removes privacy concerns out of the packwerk repository. It moves them to packwerk-privacy in this PR: rubyatscale/packwerk-extensions#1

What approach did you choose and why?

I completely removed all of the privacy related code along with tests that should be moved to the other repo.

What should reviewers focus on?

Is there something that I didn't remove that I should, or something I shouldn't have removed that I did?

Type of Change

Bugfix
New feature: Extendible checkers and validators!
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)

Users will need to include packwerk-privacy in their Gemfile and add packwerk-privacy to the require directive in packwerk.yml. USAGE.md includes more detail for how to do this.

This PR will require a major version bump, and the upgrade guide should go into a bit more detail on this.
We'll also want to couple a major version bump with the removal of references to deprecated_references.yml.

Checklist

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

rafaelfranca

When building the plugin itself, if you are copying code from this repository don't forget to include the same copyright and license in the plugin since it is a substantial copy of this project.

README.md

lib/packwerk/application_validator.rb

alexevanczuk · 2022-11-01T12:35:24Z

lib/packwerk/checker_interface.rb

@@ -0,0 +1,46 @@
+# typed: strict


This was just moved from Packwerk::ReferenceChecking::Checkers::Checker. The main reason for that is that the extensive module nesting felt like an implementation detail and now that extending the checker is a supported public API, I felt like the more concise Packwerk::CheckerInterface was an improved public API.

alexevanczuk · 2022-11-01T12:40:11Z

lib/packwerk/validator_interface/helpers.rb

@@ -0,0 +1,52 @@
+# typed: strict


This was moved from lib/packwerk/application_validator/helpers.rb, as this is now a ValidatorInterface concern. I also made these proper module methods so they can be included as helpers in the ValidatorInterface. I could also just inline them into ValidatorInterface if folks prefer.

alexevanczuk · 2022-11-01T12:42:43Z

test/fixtures/package_todo.yml

@@ -2,7 +2,7 @@
 buyers:
  "::Buyers::Document":
    violations:
-    - dependency
+    - some_checker_type


This was originally used to test when a dependency violation turned into a privacy violation. I changed it to privacy originally and then realized we could use any arbitrary string to represent a different checker's violation.

alexevanczuk · 2022-11-01T18:23:46Z

@rafaelfranca @gmcgibbon I think this is in a good state for review now!

USAGE.md

gmcgibbon · 2022-11-09T08:51:26Z

lib/packwerk/checker_interface.rb

+# frozen_string_literal: true
+
+module Packwerk
+  module CheckerInterface


Suggested change

module CheckerInterface

module CheckerBehaviour

This defines behaviour, so it technically isn't an interface, is it? Maybe AbstractChecker makes more sense? Does Sorbet have a standard for this?

According to the sorbet docs, it's an abstract class: https://sorbet.org/docs/abstract

Sorbet doesn't have a standard as far as I can see, but using the language of sorbet (i.e. AbstractChecker) does probably make sense. I've always liked including the word "interface" in my interfaces to make it clear that the user has to implement certain methods, but I think abstract signifies the same thing here. Also cc @mclark and also maybe @paracycle who may have suggestions on naming conventions for sorbet interfaces.

lib/packwerk/checker_interface.rb

lib/packwerk/validator_interface.rb

gmcgibbon · 2022-11-09T09:02:50Z

lib/packwerk/validator_interface.rb

+    extend T::Sig
+    extend T::Helpers
+
+    include Helpers


This will load helpers immediately, so there is no need to autoload it. Also curious if we should leave it up to the concrete implementation to include this if they need it?

When you say there is no need to autoload it -- do you mean replace with a vanilla require statement (since something needs to require it somehow)? I made that change.

I'm inclined to keep it because the fact that it's in a separate module is mostly just an implementation detail -- in fact I wonder if it would be better to just inline it into AbstractValidator. I think a lot of validators will need these methods (as the existing ones did) and wanted to avoid an extra step for the clients to be able to implement they validation.

That being said I'm open to changing this so the concrete implementation has to include it if you feel strongly about it!

When you say there is no need to autoload it -- do you mean replace with a vanilla require statement (since something needs to require it somehow)? I made that change.

If you tell ruby to manually autoload something (via autoload :Helpers and then include Helpers the autoload triggers immediately and defeats the purpose of using autoload. So I think we should use a require instead at the top of this file.

That being said I'm open to changing this so the concrete implementation has to include it if you feel strongly about it!

No, I think it is fine to keep, as long as we require it properly. I would say we could make it a private_constant, but if we want to document these methods it is better off as -is. I just want to make sure we're defining our initial public API well.

Gotcha -- added the require and made Helpers a private_constant!

Gotcha, thanks for the explanation. Ya know, realizing that the module Helpers is basically private and the API is on AbtractValidator, I went ahead with experimenting just inlining Helpers into AbstractValidator and I think it looks a lot better – it makes it clear what API is available for an AbstractValidator and removes this class which I think is just obscuring the available API.

gmcgibbon · 2022-11-09T09:10:27Z

lib/packwerk/cli.rb

+    end
+
+    sig { returns(PackageSet) }
+    def package_set


I had to define the package set on the run_context in parse_run to cleanup todos. I wonder if we DRY things up somehow.

I'm not sure exactly. We could have ParseRun take in a fully initialized RunContext, and then pass in the package_set from here into the initialization of RunContext. Right now RunContext is only used for parsing related functionality, so this would have us be sharing variables used in validation and parsing in a somewhat new way, and getting the package set is using package set public API anyways, so I'm not sure how to do this in a way that simplifies the design.

Let me know if you have suggestions. I also think it could be something we come back to in a separate PR since it's a structural/non-behavioral change only.

I think we would be passing the package set via this method down to where I was using it. But yeah, we can deal with later.

alexevanczuk · 2022-11-09T12:48:13Z

@gmcgibbon I believe I've responded to and/or incorporated all of your feedback!

gmcgibbon · 2022-11-10T22:52:08Z

lib/packwerk/abstract_validator.rb

+    end
+
+    sig { abstract.params(package_set: PackageSet, configuration: Configuration).returns(ApplicationValidator::Result) }
+    def call(package_set, configuration)


Suggested change

def call(package_set, configuration)

def validate(package_set, configuration)

I think naming this method will help if the object grows in complexity. Similar to how checkers respond to multiple methods. We can't substitute this for a proc or a simpler object because it needs to define permitted_keys as well so I don't see the benefit of calling this method call.

Makes a lot of sense, I'll make this change next week before merging.

gmcgibbon · 2022-11-10T22:53:45Z

lib/packwerk/abstract_validator.rb

+    def call(package_set, configuration)
+    end
+
+    sig { params(configuration: Configuration, setting: T.untyped).returns(T.untyped) }


Suggested change

sig { params(configuration: Configuration, setting: T.untyped).returns(T.untyped) }

sig { params(configuration: Configuration, setting: String).returns(T.untyped) }

If we're reading a YAML file, there's a very good chance the key will be a string. You could also use Object if you want it to be generalized.

Good call, I think going with string is good, at least until we have a use case for anything else.

gmcgibbon · 2022-11-10T22:59:17Z

lib/packwerk/abstract_validator.rb

+    class << self
+      extend T::Sig
+
+      sig { params(base: Class).void }


This isn't technically true. If we want to guarantee that these are classes we could consider making these abstract module abstract classes. We could also drop the abstract wording and call them Validator and Checker which would make AbstractChecker.all -> Checker.all which is more straightforward API in my opinion. However, I don't feel too strongly about this point so I'll leave the decision up to you. Since we've defined more behaviour in this module it seems appropriate to me that it should be an abstract class.

I'm not sure I'm totally following, but I think for this to be a sorbet interface is has to be a module. Are you saying that this in theory could be implemented by a module so it's not necessarily a class that gets passed in? If so I think that's technically true but the only supported and documented use would be including in a class.

I like checker and validator too actually -- does seem like a cleaner public API for use and the fact that it's abstract feels like an implementation detail a bit? As far as packwerk's sorbet signatures go, it's just a "checker" and "validator" so I'll change that next week before merging.

I think for this to be a sorbet interface is has to be a module

Classes should work, but they can't be interfaces. The concept of an abstract class is in other languages too. Usually, in typed languages, interfaces can't define behaviour and therefore can't be instantiated, while abstract classes are the middle-ground between an interface and a the implementation (partially defining behaviour, but also defining a contract).

The only difference is you would have to use an inherited hook instead.

but the only supported and documented use would be including in a class.

Yes, we expect it to only be included in classes, but a developer could make their own and try include that module in their concrete classes instead. They may assume it will work (but we expect a direct ancestor line from the class, so it wouldn't). An abstract class has a similar flaw if they wanted to share behaviour with subclasses though.

the fact that it's abstract feels like an implementation detail a bit?

Yes, most gem APIs don't mention concepts like that, but since we use Sorbet we walk a fine line between Ruby conventions and strongly typed language conventions.

USAGE.md

mriddle · 2024-02-28T08:41:36Z

For those interested, there is summary of the reasoning behind these changes outlined in the follow blog post:
https://railsatscale.com/2024-01-26-a-packwerk-retrospective#packwerk-as-a-privacy-enforcer

(Full details in this discussion)

Unfortunately, while easy to use, Packwerk’s privacy checks introduced several problems. Some of these were problems of implementation: the checks required a separate app/public directory for code that was meant to be public API. This broke Rails conventions on file layout by introducing a folder under app that denoted privacy level instead of architecture concepts. Confusion around where files should go resulted in new subdirectories being created for controllers and jobs, duplicating those that already existed under app. As public API, these subdirectories should have been documented and well thought-out, but Packwerk didn’t encourage this level of detail. Thus, we ended up with endless poorly-documented public code that was never meant to be public in the first place.

Yet there was a deeper problem, namely that privacy checks had transformed Packwerk into something it was never intended to be: an API design tool. Packwerk was being used to ensure that packages communicated via blessed entrypoints, whereas its original purpose was to define and enforce a dependency graph. Package A using package B’s code (even its public API) is not acceptable if package A doesn’t depend on B, yet we found developers were focusing on the design of their APIs over the dependencies in their code. This was drawing attention away from the problem the tool had been created to solve.

Given these issues, privacy checks were removed from Packwerk with the release of version 3.0.

rafaelfranca reviewed Oct 31, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

lib/packwerk/application_validator.rb Outdated Show resolved Hide resolved

alexevanczuk force-pushed the ae-pull-privacy-out-of-packwerk branch from b4d3402 to 212f417 Compare November 1, 2022 11:55

alexevanczuk commented Nov 1, 2022

View reviewed changes

alexevanczuk marked this pull request as ready for review November 1, 2022 12:44

alexevanczuk requested a review from a team as a code owner November 1, 2022 12:44

alexevanczuk requested a review from rafaelfranca November 1, 2022 12:44

alexevanczuk mentioned this pull request Nov 1, 2022

Initial extraction from packwerk codebase rubyatscale/packwerk-extensions#1

Merged

alexevanczuk force-pushed the ae-pull-privacy-out-of-packwerk branch 3 times, most recently from 085e1b6 to b41eab5 Compare November 8, 2022 12:52

gmcgibbon reviewed Nov 9, 2022

View reviewed changes

alexevanczuk requested a review from gmcgibbon November 9, 2022 12:48

alexevanczuk force-pushed the ae-pull-privacy-out-of-packwerk branch from 6fe5589 to 1896030 Compare November 10, 2022 00:58

gmcgibbon reviewed Nov 10, 2022

View reviewed changes

gmcgibbon approved these changes Nov 10, 2022

View reviewed changes

Pull privacy concerns out of packwerk

2a439d4

alexevanczuk force-pushed the ae-pull-privacy-out-of-packwerk branch from 009534e to 2a439d4 Compare November 14, 2022 13:23

alexevanczuk merged commit 72aa3cf into main Nov 14, 2022

alexevanczuk deleted the ae-pull-privacy-out-of-packwerk branch November 14, 2022 13:27

adrianna-chang-shopify mentioned this pull request Nov 22, 2022

Remove reference to privacy violations from TROUBLESHOOT.md #267

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pull privacy concerns out of packwerk #247

Pull privacy concerns out of packwerk #247

alexevanczuk commented Oct 31, 2022 •

edited

Loading

rafaelfranca left a comment

alexevanczuk Nov 1, 2022

alexevanczuk Nov 1, 2022

alexevanczuk Nov 1, 2022

alexevanczuk commented Nov 1, 2022

gmcgibbon Nov 9, 2022

alexevanczuk Nov 9, 2022

gmcgibbon Nov 9, 2022

alexevanczuk Nov 9, 2022

alexevanczuk Nov 9, 2022

gmcgibbon Nov 10, 2022 •

edited

Loading

alexevanczuk Nov 10, 2022

alexevanczuk Nov 10, 2022

gmcgibbon Nov 9, 2022

alexevanczuk Nov 9, 2022

alexevanczuk Nov 9, 2022

gmcgibbon Nov 10, 2022

alexevanczuk commented Nov 9, 2022

gmcgibbon Nov 10, 2022

alexevanczuk Nov 11, 2022

gmcgibbon Nov 10, 2022 •

edited

Loading

alexevanczuk Nov 11, 2022

gmcgibbon Nov 10, 2022

alexevanczuk Nov 11, 2022

gmcgibbon Nov 11, 2022 •

edited

Loading

gmcgibbon Nov 11, 2022

mriddle commented Feb 28, 2024 •

edited

Loading

	def call(package_set, configuration)
	def validate(package_set, configuration)

	sig { params(configuration: Configuration, setting: T.untyped).returns(T.untyped) }
	sig { params(configuration: Configuration, setting: String).returns(T.untyped) }

Pull privacy concerns out of packwerk #247

Pull privacy concerns out of packwerk #247

Conversation

alexevanczuk commented Oct 31, 2022 • edited Loading

Do not merge yet

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

rafaelfranca 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

alexevanczuk commented Nov 1, 2022

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

gmcgibbon Nov 10, 2022 • 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

Choose a reason for hiding this comment

alexevanczuk commented Nov 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmcgibbon Nov 10, 2022 • 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

gmcgibbon Nov 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mriddle commented Feb 28, 2024 • edited Loading

alexevanczuk commented Oct 31, 2022 •

edited

Loading

gmcgibbon Nov 10, 2022 •

edited

Loading

gmcgibbon Nov 10, 2022 •

edited

Loading

gmcgibbon Nov 11, 2022 •

edited

Loading

mriddle commented Feb 28, 2024 •

edited

Loading