Discussion about changes to public API

[alexevanczuk]

Hi everyone! Now that we're wrapping up allowing packwerk to be extended, let's continue the discussion from Shopify/packwerk#219 on what changes we want to make to the privacy checking functionality. I'd love to help ensure we continue to support the community, so please share any and all concerns about the following changes and we can discuss how to address them.

Here are some changes that I think might be valuable. Please share any feedback about this as well as any ideas you have that are not listed here!

Breaking Changes

Invert violations in package_todo.yml files (formerly known as deprecated_references.yml flies)

Right now package_todo.yml files show OUTBOUND privacy violations. After using privacy checking extensively at Gusto, we've found it feels a lot more sensible for package_todo.yml files to show INBOUND privacy violations. We've found that addressing privacy violations on a package is the responsibility of the publishing package, not the consuming package (who is a client of the publishing package and should not be responsible for shaping the public API of a different system). We believe this will better orient incentives and tell a better story (namely – to improve a package, you only need to look at its own package_todo.yml file.

I believe this may be blocked on upstream changes into packwerk to allow checkers to have more control over where violations show up, but I want to gather feedback before pursuing that.

Remove the "enforce privacy for specific constants" feature

At Gusto, we do not use this feature because we find it a lot more valuable to think about packages as private by default rather than public by default. Using this features establishes a public by default mentality, since everything is public except for the listed constants. I believe that a focus on public by default as the golden path moving forward would allow us to remove some extra complexity, but I want to check in with folks if they're using this feature and find it valuable.

Two alternatives to removing this:

1. If an individual or org is passionate about maintaining this, it could be its own plugin.
2. Moving the list of private constants to a separate package.yml key (e.g. private_constants) would simplify the maintenance of the enforce_privacy setting and tidy things up a bit.

Remove user defined public paths

Are folks using this one? At Gusto, we exclusively use app/public, which is the default. I wanted to check in if folks are using this and found it helpful. I don't see an issue in maintaining this, but if folks are not using it and we're doing a major version bump, it would be nice to remove.

New features

Sigil based public API promotion

We'd like a new way to declare a constant (i.e. a file) as public. Having a dedicated "public" folder (i.e. coupling public API designation to file location) has caused some issues and pain for folks. There are some tools out there, Rails included, which expect certain things in certain places, e.g. services.

This is especially relevant when it comes to nested packs (in the world of stimpack), since we'd like a way for a child pack to "promote" its public API to the parent pack. Here are some API ideas:
For making something public to the same pack

# public_api

For making something public to a parent pack

# public_api: packs/parent_apck

Nested Pack Support

We want to optionally allow nested packs to ignore privacy violations within pack groups.

[flavorjones]

After using privacy checking extensively at Gusto, we've found it feels a lot more sensible for package_todo.yml files to show INBOUND privacy violations. We've found that addressing privacy violations on a package is the responsibility of the publishing package, not the consuming package (who is a client of the publishing package and should not be responsible for shaping the public API of a different system).

This is an interesting insight! Can you share a story from Gusto to help folks understand how this plays out?

[alexevanczuk]

Hey @flavorjones – happy to.

The general story that we see play out frequently is that someone in one domain (say “payroll,” a common Gusto domain) needs to use API from another domain (say “payments,” another common Gusto domain). The payroll eng gets a privacy violation on payments, but there is no public API alternative yet. They get a violation in their package, and feel disempowered to create an abstraction for the payments behavior they’re looking for, because they are not domain experts and they would not be set up to own that API long-term. In this case, we’d want the payments team to know (and be responsible for) building that abstraction.

Later on, when payments does create that abstraction, and ideally migrate callers, it feels like that responsibility for using that API should shift to the consumer (payroll). After payments has done this, they can lock down their private API, either by enabling strict mode (which we implement internally at Gusto and thought would be interesting to upstream to packwerk) OR by using private_constant on their private API, which helps ensure consumers use the public API on an ongoing basis (or request new API if one does not exist for their use case).

[mclark]

@alexevanczuk thanks for starting this discussion and for all your hard work to make this possible!

Invert violations in package_todo.yml files

💯 I completely agree and this matches up with my experience in the GitHub monolith.

The only risk I can see is if teams add calls that violate privacy even though there is a public method that would suit their purposes. This would add a deprecation to the publishing team's package even though they've done everything right. This might just be a matter of people communicating though rather than a technical problem. The publishing package could also set their privacy enforcement to "strict" which would at least allow them to be alerted via CODEOWNERS if someone tries to drop it to "true".

Remove the "enforce privacy for specific constants" feature

Agreed again. We haven't seen any value from this feature other than testing. Removing this feature really clarifies the purpose of the enforce_privacy option which I like.

Remove user defined public paths

We also use app/public at GitHub but that hasn't always been the case and I can imagine scenarios where we might use different directories. Utility packages, for instance, might not really need an app directory at all. I for one say keep this feature around, at least for now.

Sigil based public API promotion

This one is interesting. Personally, I really appreciate the clear demarkation of having a "public" folder separate from a "private" folder. Privacy is very important and feels "right" to leverage the directory structure to communicate it. That being said, Rails has already decided (eons ago) that directories are more like ways of "tagging" Ruby files to indicate their purpose and makes certain assumptions about these "tags". app/controllers will always contain files that act as controllers for instance.

Rails was here first and we need to respect that. We're not going to get a packs/pack1/public/my_controller.rb and a packs/pack1/public/my_model.rb at this point. So putting privacy in these files as sigils instead makes sense. That being said, I don't plan on leveraging this feature at GitHub, at least in the foreseeable future. Mixing public and private code in the same directory will mean we will need to rely on generated documentation and possibly IDE plugins to determine the visibility of the constants which I don't think is ideal. But maybe that perspective will change as time goes by and my understanding grows.

[alexevanczuk]

Hey thanks for all this feedback. Glad to see you're generally aligned.

I agree about the inversion of violations. Also note that if someone adds a violation without strict mode, the owning team would get notified via CODEOWNERS too (if their package_todo.yml files are properly owned by the package owner), which is a nice bonus.

Keeping user defined public paths sounds fine!

I also really like the idea of a separate public folder. To your point about generated documentation: at Gusto, we actually use a souped-up version of the following command:

bundle exec yard doc \
  --output-dir tmp/docs \
  --plugin sorbet \
  --no-private \
  --template-path script/yard \
  ./**/app/public/**/*.rb

To generate yard-docs and upload them to an internal bucket so consumers can look at auto-generated docs for public API. However, this is extra tooling to maintain and extra steps to take (for consumers of public API), which is a big downside.

I think suffice to say, a public folder and a sigil-based approach both have their pros and cons, and regardless of what we do with sigil-based public APIs, it doesn't make sense to remove a location-based public API strategy (i.e. clients should be able to use either).

[Skipants]

At Gusto, we do not use this feature because we find it a lot more valuable to think about packages as private by default rather than public by default.

I feel the same way. Is there any appetite for inverting the idea of the list of private_constant to public_constants? It seems to align with what you say here. It enforces private-by-default and makes listing public constants an explicit choice.

[alexevanczuk]

@Skipants Apologies for the delay in getting back to you here.

Allowing constants to be public if they're in the public folder or in a list of public_constants makes sense to me! Want to try to implement and send a PR? Happy to help.

[Skipants]

I could definitely tackle that!

I think it would be an addition to packwerk/privacy/checker since they both enforce privacy by default. Then "simply" implement public_constants as a list of constants that are public. I think it can be made to co-exist fine with the current public directory and the sigil-based approach you outlined, as all three sets can be merged to create the public interface. I'm sure the most common usage would be to just go with one approach. But, by ensuring it plays nicely with the other ways of declaring things public, it lets people migrate from one to the other based on their needs..

Let me know if you foresee any issues with that plan.

[alexevanczuk]

That sounds good to me!