Packwerk checks for stale violations #61

cindygshopify · 2020-11-03T22:48:49Z

What are you trying to accomplish?

Adding feature for Packwerk checking stale violations. This should be incorporated into CI in the future to make sure that PRs don't pass CI if there are stale violations. Linked issue here: #12

What approach did you choose and why?

We had to refactor the code a bit to create this feature. We needed a lot of functionality in the UpdatingDeprecatedReferences.rb class but putting the change in that class didn't seem right. So CacheDeprecatingReferences.rb was created as a parent class that both UpdatingDeprecatedReferences.rb and DetectStaleDeprecatedReferences.rb now inherit from. Functionality common to both now belongs in CacheDeprecatingReferences.rb and functionality specific to updating vs detecting stale violations are now in their respective classes.

When executing the command, Packwerk will go through all the loaded files and check for violations in the files which it then stores in DetectStaleDeprecatedReferences.rb. At the end when all files were processed, we compare the deprecated_references.yml file of each component loaded to the actual violations generated. If there are references in deprecated_references.yml that aren't in the generated violations, then there are stale violations. The program will exits with status code 1. If there are no stale violations, program exits with status code 0.

What should reviewers focus on?

Any issues with the code refactor? Are there stale violation cases we may have missed with this code change?

Tophatting

My tophatting instructions use Packwerk in the shopify/shopify repo, but you can use another repo if you prefer

Pull this branch into local
Change the gemfile of local shopify/shopify repo to point to this branch
Modify dev.yml of shopify/shopify to include the detect-stale violations command like so:

4) Generate a stale violation in shopify/shopify.

You can do this by changing package.yml of a component, enforcing dependencies on it and run Packwerk update to generate a deprecated_references.yml for that component which has boundary violations.
After you have a list of dependency violations for that component, you can add the component which the violation is coming from as a dependency in the package.yml of your component

Run dev packages detect-stale-violations path\to\your\component
Check that it outputs stale violations were made
Run $? to check status of last command (0 if no stale violations, 1 if there were stale violations)

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Code refactor (non-breaking change that doesn't add functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Additional Release Notes

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

It is safe to simply rollback this change.

tomstuart

Thank you for doing this, Cindy!

No showstoppers here but I had a few ideas while reading through your code so I’ve added some comments for now in case you’d like to discuss them. Let me know what you think.

lib/packwerk/cache_deprecated_references.rb

lib/packwerk/updating_deprecated_references.rb

lib/packwerk/deprecated_references.rb

lib/packwerk/detect_stale_deprecated_references.rb

lib/packwerk/updating_deprecated_references.rb

test/unit/deprecated_references_test.rb

test/unit/detect_stale_deprecated_references_test.rb

lib/packwerk/cli.rb

doodzik · 2020-11-04T18:19:49Z

lib/packwerk/cache_deprecated_references.rb

@@ -0,0 +1,45 @@
+# typed: true


I think it would be good that new files to this codebase are in strict mode. A lot of the benefits from sorbet only arise if we have enough type information available.

While that would be nice, these methods are extracted from what used to be UpdatingDeprecatedReferences class and unfortunately I don't have enough context to know what the signatures for these classes would be.

I think it is important that we continue adopting sorbet in this codebase because it will improve the code's maintainability over time. It is inconvenient to look up the signatures, but it is beneficial in the long term.

Some classes in this codebase are hard to add typing, but in this case, the type signatures are pretty straightforward. I've added the signatures to the missing methods below, so once adopted, you should be able to change the class to strict:

sig { params(root_path: String, deprecated_references: T::Hash[String, Packwerk::DeprecatedReferences]) } def initialize(root_path, deprecated_references = {}) @root_path = root_path @deprecated_references = T.let(deprecated_references, T::Hash[String, Packwerk::DeprecatedReferences]) end sig { params(package: Package).returns(Packwerk::DeprecatedReferences) } def deprecated_references_for(package) @deprecated_references[package] ||= Packwerk::DeprecatedReferences.new( package, deprecated_references_file_for(package), ) end sig { params(package: Package).returns(String) } def deprecated_references_file_for(package) File.join(@root_path, package.name, "deprecated_references.yml") end

I attempted to do this as it seemed like it would be low hanging fruit we could ship with this PR, but these signatures generated some really weird typecheck errors that should be further investigated in another PR. For instance, in deprecated_references_for package param seems to be both a String and Package. It's a little weird and I really think necessitates further investigation.

lib/packwerk/detect_stale_deprecated_references.rb

…eferences class to support other functionality such as detecting stale deprecations

lib/packwerk/cli.rb

lib/packwerk/deprecated_references.rb

wildmaples

Yay this is great work!

test/unit/cache_deprecated_references_test.rb

wildmaples · 2020-11-09T22:50:23Z

lib/packwerk/deprecated_references.rb

@@ -7,7 +7,6 @@
 require "packwerk/reference"
 require "packwerk/reference_lister"
 require "packwerk/violation_type"
-


lib/packwerk/cli.rb

lib/packwerk/cache_deprecated_references.rb

cindygshopify · 2020-11-12T18:15:21Z

I've pushed one more commit that moves the detect_stale_violations function that was in cli.rb into it's own class so we can get some separation of concerns in the Packwerk codebase. This required some refactoring of cli.rb as well by putting some common functions into a module. I've also updated the code to reflect some of the above mentioned comments, so please take a look. There are some sorbet signatures I have not added as it negatively affects the tests that I've written. The solution to that requires even more refactoring in my opinion. In the interest of time, I'd prefer not to drag out this PR more if it is related to further refactoring. I think this is better served by creating new GitHub issues around that so we can avoid scope creep.

I'll also go ahead and rewrite some of the history so that the commits make a little more sense but after this PR is in a good enough state to ship.

tomstuart · 2020-11-13T15:21:19Z

Thank you Cindy, the code looks good to me. My laptop is (very slowly) tophatting it now, then I’ll approve 👍

tomstuart · 2020-11-13T17:45:46Z

packwerk update works fine for this branch, so that looks good.

I’m sorry but I couldn’t successfully tophat dev packages detect-stale-violations in Shopify/shopify. When I tried to run it I got the error:

lib/packwerk/cli.rb:192:in `detect_stale_violations': uninitialized constant Packwerk::Cli::DetectStaleViolationsCommand (NameError)

I can fix that by putting require "packwerk/commands/detect_stale_violations_command" at the top of lib/packwerk/cli.rb, but then I get a different error:

lib/packwerk/commands/detect_stale_violations_command.rb:33:in `block (2 levels) in run': private method `file_processor' called for #<Packwerk::RunContext> (NoMethodError)

I could fix that by making RunContext#file_processor public, but rather than continue chasing these I thought I’d report back and let you know it wasn’t working right. Was it working fine before? What’s changed?

cindygshopify · 2020-11-13T17:48:45Z

Was it working fine before? What’s changed?

It was. I tested before all the refactoring shenanigans so that might've broken something. I figured it would have been covered by the test cases I wrote, but it seems like it wasn't. I'll take another look

cindygshopify · 2020-11-13T18:37:52Z

There was a minor change in another file that broke the functionality of detect-stale-violations, I just pushed with an amended change. Tophatting should work now.

wildmaples · 2020-11-13T21:44:33Z

There was a minor change in another file that broke the functionality of detect-stale-violations, I just pushed with an amended change. Tophatting should work now.

What was the issue and how did you solve it?

Edit: Asking because if you thought the test should have caught it, perhaps we are missing a test?

cindygshopify · 2020-11-13T22:12:43Z

There was a minor change in another file that broke the functionality of detect-stale-violations, I just pushed with an amended change. Tophatting should work now.

What was the issue and how did you solve it?

RunContext#file_processor was a public function that was being used in DetectStaleViolationsCommand. However due to a refactor outside of this PR, it got changed to be a private function. However since this function was previously being used outside of it's own class, another function was written to replace the functionality of the function calls outside its' class. Thus I replaced RunContext#file_processor with RunContext#process_file

wildmaples

🎩 Tophatted on Core and it works!!! 🎉

This is a huge (and much needed) change, but I would prefer to include it in release v1.0.3 instead of v1.0.2.

Reason is because we are currently fixing a change for v1.0.2 and if this change is merged and for some unexpected reason breaks, we'd have two back-to-back broken versions.

cindygshopify · 2020-11-13T22:32:06Z

Reason is because we are currently fixing a change for v1.0.2 and if this change is merged and for some unexpected reason breaks, we'd have two back-to-back broken versions.

Okay. Will we need to hold off merging this for a while then or would that not matter and just be something to do noted by the release manager when doing the releases?

doodzik · 2020-11-16T17:29:25Z

lib/packwerk/cli.rb

-        @progress_formatter.mark_as_failed
-      end
-    end
-


doodzik

Nice job on the feature and the cleanup 🙌
I think this PR is a good example of leaving the codebase in a better state than before 🎉

This is a huge (and much needed) change, but I would prefer to include it in release v1.0.3 instead of v1.0.2.

@wildmaples shouldn't this be a minor rather than patch version change? (v1.0.1 -> v1.1.0), because we are adding new non-breaking functionality?

MINOR version when you add functionality in a backward-compatible manner, and
https://semver.org/

…class as well as associated unit tests

…eck for stale violations as a reference lister. In addition wrote unit tests for this class

…y the functionality of dectect-stale-violations function in cli.rb

exterm · 2020-11-19T22:15:24Z

Is anyone following up by using this to fix the stale violations check that is already in core CI?

cindygshopify · 2020-11-23T14:49:42Z

@exterm I'm doing that

Have privacy checker check for shitlisted references

cindygshopify marked this pull request as ready for review November 3, 2020 22:55

cindygshopify requested a review from a team as a code owner November 3, 2020 22:55

tomstuart reviewed Nov 4, 2020

View reviewed changes

doodzik reviewed Nov 4, 2020

View reviewed changes

lib/packwerk/detect_stale_deprecated_references.rb Show resolved Hide resolved

cindygshopify force-pushed the cg_packwerk_checks_for_stale_violations branch 2 times, most recently from 5634f4f to 0083aa2 Compare November 9, 2020 20:23

extract CacheDeprecatedReferences superclass from UpdatingDeprecatedR…

c1ca84e

…eferences class to support other functionality such as detecting stale deprecations

cindygshopify force-pushed the cg_packwerk_checks_for_stale_violations branch from 0083aa2 to 7d110ef Compare November 9, 2020 21:20

wildmaples reviewed Nov 9, 2020

View reviewed changes

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

wildmaples reviewed Nov 9, 2020

View reviewed changes

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

cindygshopify force-pushed the cg_packwerk_checks_for_stale_violations branch from 7d110ef to fe49aee Compare November 9, 2020 22:47

wildmaples reviewed Nov 9, 2020

View reviewed changes

doodzik reviewed Nov 10, 2020

View reviewed changes

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

exterm reviewed Nov 10, 2020

View reviewed changes

lib/packwerk/cache_deprecated_references.rb Show resolved Hide resolved

cindygshopify force-pushed the cg_packwerk_checks_for_stale_violations branch 3 times, most recently from 5653fc5 to 32db927 Compare November 12, 2020 17:51

cindygshopify force-pushed the cg_packwerk_checks_for_stale_violations branch from 32db927 to 6a95712 Compare November 13, 2020 18:35

wildmaples approved these changes Nov 13, 2020

View reviewed changes

doodzik reviewed Nov 16, 2020

View reviewed changes

lib/packwerk/cli.rb

@progress_formatter.mark_as_failed

end

end

Copy link

Contributor

doodzik Nov 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

doodzik approved these changes Nov 16, 2020

View reviewed changes

cindygshopify added 3 commits November 16, 2020 13:53

add method to check for stale violations within DeprecatedReferences …

f0d3d91

…class as well as associated unit tests

created DetectStaleDeprecatedReferences class which can be used to ch…

1e90c53

…eck for stale violations as a reference lister. In addition wrote unit tests for this class

created new class DetectStaleViolationsCommand in order to test easil…

e6ee950

…y the functionality of dectect-stale-violations function in cli.rb

cindygshopify force-pushed the cg_packwerk_checks_for_stale_violations branch from 6a95712 to e6ee950 Compare November 16, 2020 18:55

cindygshopify merged commit 50262e8 into main Nov 17, 2020

cindygshopify deleted the cg_packwerk_checks_for_stale_violations branch November 17, 2020 15:10

cindygshopify restored the cg_packwerk_checks_for_stale_violations branch November 23, 2020 14:49

cindygshopify deleted the cg_packwerk_checks_for_stale_violations branch November 23, 2020 14:49

cindygshopify temporarily deployed to rubygems November 24, 2020 21:28 Inactive

wildmaples mentioned this pull request Dec 22, 2020

Fail check if shitlisted violation is removed #12

Closed

shioyama pushed a commit that referenced this pull request May 21, 2021

Merge pull request #61 from Shopify/add-privacy-shitlist

30c6494

Have privacy checker check for shitlisted references

rafaelfranca mentioned this pull request Nov 7, 2022

Cleanup CLI #255

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Packwerk checks for stale violations #61

Packwerk checks for stale violations #61

cindygshopify commented Nov 3, 2020 •

edited

Loading

tomstuart left a comment

doodzik Nov 4, 2020

cindygshopify Nov 9, 2020

doodzik Nov 10, 2020 •

edited

Loading

cindygshopify Nov 12, 2020

wildmaples left a comment

wildmaples Nov 9, 2020

cindygshopify commented Nov 12, 2020 •

edited

Loading

tomstuart commented Nov 13, 2020

tomstuart commented Nov 13, 2020

cindygshopify commented Nov 13, 2020

cindygshopify commented Nov 13, 2020

wildmaples commented Nov 13, 2020 •

edited

Loading

cindygshopify commented Nov 13, 2020

wildmaples left a comment •

edited

Loading

cindygshopify commented Nov 13, 2020

doodzik Nov 16, 2020

doodzik left a comment •

edited

Loading

exterm commented Nov 19, 2020

cindygshopify commented Nov 23, 2020 •

edited

Loading

Packwerk checks for stale violations #61

Packwerk checks for stale violations #61

Conversation

cindygshopify commented Nov 3, 2020 • edited Loading

What are you trying to accomplish?

What approach did you choose and why?

What should reviewers focus on?

Tophatting

Type of Change

Additional Release Notes

Checklist

tomstuart left a comment

Choose a reason for hiding this comment

doodzik Nov 4, 2020

Choose a reason for hiding this comment

cindygshopify Nov 9, 2020

Choose a reason for hiding this comment

doodzik Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

cindygshopify Nov 12, 2020

Choose a reason for hiding this comment

wildmaples left a comment

Choose a reason for hiding this comment

wildmaples Nov 9, 2020

Choose a reason for hiding this comment

cindygshopify commented Nov 12, 2020 • edited Loading

tomstuart commented Nov 13, 2020

tomstuart commented Nov 13, 2020

cindygshopify commented Nov 13, 2020

cindygshopify commented Nov 13, 2020

wildmaples commented Nov 13, 2020 • edited Loading

cindygshopify commented Nov 13, 2020

wildmaples left a comment • edited Loading

Choose a reason for hiding this comment

cindygshopify commented Nov 13, 2020

doodzik Nov 16, 2020

Choose a reason for hiding this comment

doodzik left a comment • edited Loading

Choose a reason for hiding this comment

exterm commented Nov 19, 2020

cindygshopify commented Nov 23, 2020 • edited Loading

cindygshopify commented Nov 3, 2020 •

edited

Loading

doodzik Nov 10, 2020 •

edited

Loading

cindygshopify commented Nov 12, 2020 •

edited

Loading

wildmaples commented Nov 13, 2020 •

edited

Loading

wildmaples left a comment •

edited

Loading

doodzik left a comment •

edited

Loading

cindygshopify commented Nov 23, 2020 •

edited

Loading