Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add bin/packs lint_deprecated_references command #55

Merged
merged 5 commits into from
Nov 18, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Gemfile.lock
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
PATH
remote: .
specs:
use_packwerk (0.65.0)
use_packwerk (0.66.0)
code_ownership
colorize
packwerk (>= 2.2.1)
Expand Down
36 changes: 36 additions & 0 deletions lib/use_packwerk.rb
Original file line number Diff line number Diff line change
Expand Up @@ -245,4 +245,40 @@ def self.get_offenses_for_files_by_package(files)
Private::PackwerkWrapper.packwerk_cli_execute_safely(argv, formatter)
formatter.aggregated_offenses.compact
end

sig { void }
def self.lint_deprecated_references!
contents_before = Private.get_deprecated_references_contents
UsePackwerk.execute(['update-deprecations'])
contents_after = Private.get_deprecated_references_contents
diff = Private.diff_deprecated_references_yml(contents_before, contents_after)

if diff == ''
# No diff generated by `update-deprecations`
exit 0
else
output = <<~OUTPUT
All `deprecated_references.yml` files must be up-to-date and that no diff is generated when running `bin/packwerk update-deprecations`.
This helps ensure a high quality signal in other engineers' PRs when inspecting new violations by ensuring there are no unrelated changes.

There are three main reasons there may be a diff:
1) Most likely, you may have stale violations, meaning there are old violations that no longer apply.
2) You may have some sort of auto-formatter set up somewhere (e.g. something that reformats YML files) that is, for example, changing double quotes to single quotes. Ensure this is turned off for these auto-generated files.
3) You may have edited these files manually. It's recommended to use the `bin/packwerk update-deprecations` command to make changes to `deprecated_references.yml` files.

In all cases, you can run `bin/packwerk update-deprecations` to update these files.

Here is the diff generated after running `update-deprecations`:
```
#{diff}
```

OUTPUT

puts output
UsePackwerk.config.on_deprecated_references_lint_failure.call(output)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

At Gusto, we can configure this like this:

--- /dev/null
+++ b/config/use_packwerk.rb
@@ -0,0 +1,7 @@
+require 'gusto_packwerk'
+
+module GustoPackwerk
+  UsePackwerk.configure do |config|
+    config.on_deprecated_references_lint_failure = -> (output) { Private.post_buildkite_annotation(output) }
+  end
+end

I don't love this exactly... some alternative ideas were to have this method return a Result type with the output, and we could just do whatever we want with that result. However, I liked the idea that we can use the vanilla bin stub (bin/packs lint_deprecated_references) without needing to orchestrate anything else.


exit 1
end
end
end
6 changes: 6 additions & 0 deletions lib/use_packwerk/cli.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,5 +77,11 @@ def move_to_parent(parent_name, pack_name)
per_file_processors: [UsePackwerk::RubocopPostProcessor.new, UsePackwerk::CodeOwnershipPostProcessor.new]
)
end

desc 'lint_deprecated_references', 'Ensures `deprecated_references.yml` files are up to date'
sig { void }
def lint_deprecated_references
UsePackwerk.lint_deprecated_references!
end
end
end
8 changes: 8 additions & 0 deletions lib/use_packwerk/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,18 @@ class Configuration
sig { returns(UserEventLogger) }
attr_accessor :user_event_logger

OnDeprecatedReferencesLintFailure = T.type_alias do
T.proc.params(output: String).void
end

sig { returns(OnDeprecatedReferencesLintFailure) }
attr_accessor :on_deprecated_references_lint_failure

sig { void }
def initialize
@enforce_dependencies = T.let(default_enforce_dependencies, T::Boolean)
@user_event_logger = T.let(DefaultUserEventLogger.new, UserEventLogger)
@on_deprecated_references_lint_failure = T.let(->(output) {}, OnDeprecatedReferencesLintFailure)
end

sig { returns(T::Boolean) }
Expand Down
46 changes: 46 additions & 0 deletions lib/use_packwerk/private.rb
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,52 @@ def self.bust_cache!
# otherwise `PackageProtections.config` will attempt to reload the client configuratoin.
@loaded_client_configuration = false
end

sig { returns(T::Hash[String, String]) }
def self.get_deprecated_references_contents
deprecated_references = {}
ParsePackwerk.all.each do |package|
deprecated_references_yml = ParsePackwerk::DeprecatedReferences.for(package).pathname
if deprecated_references_yml.exist?
deprecated_references[deprecated_references_yml.to_s] = deprecated_references_yml.read
end
end

deprecated_references
end

DeprecatedReferencesFiles = T.type_alias do
T::Hash[String, T.nilable(String)]
end

sig { params(before: DeprecatedReferencesFiles, after: DeprecatedReferencesFiles).returns(String) }
def self.diff_deprecated_references_yml(before, after)
dir_containing_contents_before = Dir.mktmpdir
dir_containing_contents_after = Dir.mktmpdir
begin
write_deprecated_references_to_tmp_folder(before, dir_containing_contents_before)
write_deprecated_references_to_tmp_folder(after, dir_containing_contents_after)

diff = `diff -r #{dir_containing_contents_before}/ #{dir_containing_contents_after}/`
# For ease of reading, sub out the tmp directory from the diff
diff.gsub(dir_containing_contents_before, '').gsub(dir_containing_contents_after, '')
ensure
FileUtils.remove_entry dir_containing_contents_before
FileUtils.remove_entry dir_containing_contents_after
end
end

sig { params(deprecated_references_files: DeprecatedReferencesFiles, tmp_folder: String).void }
def self.write_deprecated_references_to_tmp_folder(deprecated_references_files, tmp_folder)
deprecated_references_files.each do |filename, contents|
next if contents.nil?

tmp_folder_pathname = Pathname.new(tmp_folder)
temp_deprecated_references_yml = tmp_folder_pathname.join(filename)
FileUtils.mkdir_p(temp_deprecated_references_yml.dirname)
temp_deprecated_references_yml.write(contents)
end
end
end

private_constant :Private
Expand Down
133 changes: 133 additions & 0 deletions spec/use_packwerk_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1292,6 +1292,139 @@ def expect_files_to_not_exist(files)
end
end

describe 'lint_deprecated_references' do
let(:args) { ['lint_deprecated_references'] }

context 'no diff after running update-deprecations' do
it 'exits successfully' do
expect_any_instance_of(Packwerk::Cli).to receive(:execute_command).with(['update-deprecations'])
expect(UsePackwerk).to receive(:exit).with(0)
expect(UsePackwerk).to_not receive(:puts)
UsePackwerk.lint_deprecated_references!
end
end

context 'some stale violations removed after running update-deprecations' do
it 'exits in a failure' do
write_file('packs/my_pack/package.yml', <<~CONTENTS)
enforce_privacy: true
enforce_dependnecy: true
CONTENTS

write_file('packs/my_pack/deprecated_references.yml', <<~CONTENTS)
---
packs/my_other_pack:
"::SomeConstant":
violations:
- privacy
files:
- packs/my_pack/app/services/my_pack.rb
- packs/my_pack/app/services/my_pack_2.rb
CONTENTS

expect_any_instance_of(Packwerk::Cli).to receive(:execute_command).with(['update-deprecations']) do
write_file('packs/my_pack/deprecated_references.yml', <<~CONTENTS)
---
packs/my_other_pack:
"::SomeConstant":
violations:
- privacy
files:
- packs/my_pack/app/services/my_pack.rb
CONTENTS
end

expect(UsePackwerk).to receive(:puts).with(<<~EXPECTED)
All `deprecated_references.yml` files must be up-to-date and that no diff is generated when running `bin/packwerk update-deprecations`.
This helps ensure a high quality signal in other engineers' PRs when inspecting new violations by ensuring there are no unrelated changes.

There are three main reasons there may be a diff:
1) Most likely, you may have stale violations, meaning there are old violations that no longer apply.
2) You may have some sort of auto-formatter set up somewhere (e.g. something that reformats YML files) that is, for example, changing double quotes to single quotes. Ensure this is turned off for these auto-generated files.
3) You may have edited these files manually. It's recommended to use the `bin/packwerk update-deprecations` command to make changes to `deprecated_references.yml` files.

In all cases, you can run `bin/packwerk update-deprecations` to update these files.

Here is the diff generated after running `update-deprecations`:
```
diff -r /packs/my_pack/deprecated_references.yml /packs/my_pack/deprecated_references.yml
8d7
< - packs/my_pack/app/services/my_pack_2.rb

```

EXPECTED

expect(UsePackwerk).to receive(:exit).with(1)
UsePackwerk.lint_deprecated_references!
end
end

context 'some formatting changes after running update-deprecations' do
it 'exits in a failure' do
callback_invocation = false
UsePackwerk.configure do |config|
config.on_deprecated_references_lint_failure = ->(output) { callback_invocation = output }
end
write_file('packs/my_pack/package.yml', <<~CONTENTS)
enforce_privacy: true
enforce_dependnecy: true
CONTENTS

write_file('packs/my_pack/deprecated_references.yml', <<~CONTENTS)
---
packs/my_other_pack:
'::SomeConstant':
violations:
- privacy
files:
- packs/my_pack/app/services/my_pack.rb
- packs/my_pack/app/services/my_pack_2.rb
CONTENTS

expect_any_instance_of(Packwerk::Cli).to receive(:execute_command).with(['update-deprecations']) do
write_file('packs/my_pack/deprecated_references.yml', <<~CONTENTS)
---
packs/my_other_pack:
"::SomeConstant":
violations:
- privacy
files:
- packs/my_pack/app/services/my_pack.rb
- packs/my_pack/app/services/my_pack_2.rb
CONTENTS
end

expect(UsePackwerk).to receive(:puts).with(<<~EXPECTED)
All `deprecated_references.yml` files must be up-to-date and that no diff is generated when running `bin/packwerk update-deprecations`.
This helps ensure a high quality signal in other engineers' PRs when inspecting new violations by ensuring there are no unrelated changes.

There are three main reasons there may be a diff:
1) Most likely, you may have stale violations, meaning there are old violations that no longer apply.
2) You may have some sort of auto-formatter set up somewhere (e.g. something that reformats YML files) that is, for example, changing double quotes to single quotes. Ensure this is turned off for these auto-generated files.
3) You may have edited these files manually. It's recommended to use the `bin/packwerk update-deprecations` command to make changes to `deprecated_references.yml` files.

In all cases, you can run `bin/packwerk update-deprecations` to update these files.

Here is the diff generated after running `update-deprecations`:
```
diff -r /packs/my_pack/deprecated_references.yml /packs/my_pack/deprecated_references.yml
3c3
< '::SomeConstant':
---
> "::SomeConstant":

```

EXPECTED

expect(UsePackwerk).to receive(:exit).with(1)
UsePackwerk.lint_deprecated_references!
expect(callback_invocation).to include('All `deprecated_references.yml` files must be up-to-date')
end
end
end

# This will soon be moved into `query_packwerk`
describe 'query_packwerk' do
before do
Expand Down
2 changes: 1 addition & 1 deletion use_packwerk.gemspec
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
Gem::Specification.new do |spec|
spec.name = 'use_packwerk'
spec.version = '0.65.0'
spec.version = '0.66.0'
spec.authors = ['Gusto Engineers']
spec.email = ['dev@gusto.com']

Expand Down