From 50d3c6f14776410b2a947faf4f246b6b5a98c37a Mon Sep 17 00:00:00 2001 From: Teal Stannard Date: Fri, 17 Jan 2025 14:01:15 -0800 Subject: [PATCH 1/2] WIP start adding var to specifiy bin/pks vs bin/packwerk Co-authored-by: Ashley Willard --- README.md | 4 +-- .../check/default_formatter.rb | 5 +-- .../check/offenses_formatter.rb | 5 +-- .../danger_package_todo_yml_changes.rb | 8 +++-- .../update/default_formatter.rb | 4 +-- .../update/offenses_formatter.rb | 5 +-- .../danger_package_todo_yml_changes_spec.rb | 32 +++++++++++++++++-- 7 files changed, 48 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index a878d60..477d94e 100644 --- a/README.md +++ b/README.md @@ -43,8 +43,8 @@ class MyFormatter extend T::Sig include DangerPackwerk::Check::OffensesFormatter # Packwerk::ReferenceOffense: https://github.com/Shopify/packwerk/blob/main/lib/packwerk/reference_offense.rb - sig { override.params(offenses: T::Array[Packwerk::ReferenceOffense], repo_link: String, org_name: String).returns(String) } - def format_offenses(offenses, repo_link, org_name) + sig { override.params(offenses: T::Array[Packwerk::ReferenceOffense], repo_link: String, org_name: String, modularization_library: String ).returns(String) } + def format_offenses(offenses, repo_link, org_name, modularization_library) # your logic here end end diff --git a/lib/danger-packwerk/check/default_formatter.rb b/lib/danger-packwerk/check/default_formatter.rb index e00308f..06838a1 100644 --- a/lib/danger-packwerk/check/default_formatter.rb +++ b/lib/danger-packwerk/check/default_formatter.rb @@ -21,10 +21,11 @@ def initialize(custom_help_message: nil) override.params( offenses: T::Array[Packwerk::ReferenceOffense], repo_link: String, - org_name: String + org_name: String, + modularization_library: String ).returns(String) end - def format_offenses(offenses, repo_link, org_name) + def format_offenses(offenses, repo_link, org_name, modularization_library: 'packwerk') reference_offense = T.must(offenses.first) violation_types = offenses.map(&:violation_type) referencing_file = reference_offense.reference.relative_path diff --git a/lib/danger-packwerk/check/offenses_formatter.rb b/lib/danger-packwerk/check/offenses_formatter.rb index 2cb5893..5b3a539 100644 --- a/lib/danger-packwerk/check/offenses_formatter.rb +++ b/lib/danger-packwerk/check/offenses_formatter.rb @@ -14,10 +14,11 @@ module OffensesFormatter abstract.params( offenses: T::Array[Packwerk::ReferenceOffense], repo_link: String, - org_name: String + org_name: String, + modularization_library: String ).returns(String) end - def format_offenses(offenses, repo_link, org_name); end + def format_offenses(offenses, repo_link, org_name, modularization_library: 'packwerk'); end end end end diff --git a/lib/danger-packwerk/danger_package_todo_yml_changes.rb b/lib/danger-packwerk/danger_package_todo_yml_changes.rb index b44ad74..4511618 100644 --- a/lib/danger-packwerk/danger_package_todo_yml_changes.rb +++ b/lib/danger-packwerk/danger_package_todo_yml_changes.rb @@ -30,7 +30,8 @@ class DangerPackageTodoYmlChanges < Danger::Plugin before_comment: BeforeComment, max_comments: Integer, violation_types: T::Array[String], - root_path: T.nilable(String) + root_path: T.nilable(String), + modularization_library: String ).void end def check( @@ -38,7 +39,8 @@ def check( before_comment: DEFAULT_BEFORE_COMMENT, max_comments: DEFAULT_MAX_COMMENTS, violation_types: DEFAULT_VIOLATION_TYPES, - root_path: nil + root_path: nil, + modularization_library: 'packwerk' ) offenses_formatter ||= Update::DefaultFormatter.new repo_link = github.pr_json[:base][:repo][:html_url] @@ -62,7 +64,7 @@ def check( location = T.must(violations.first).file_location markdown( - offenses_formatter.format_offenses(violations, repo_link, org_name), + offenses_formatter.format_offenses(violations, repo_link, org_name, modularization_library: modularization_library), line: location.line_number, file: git_filesystem.convert_to_filesystem(location.file) ) diff --git a/lib/danger-packwerk/update/default_formatter.rb b/lib/danger-packwerk/update/default_formatter.rb index b557dcf..804081e 100644 --- a/lib/danger-packwerk/update/default_formatter.rb +++ b/lib/danger-packwerk/update/default_formatter.rb @@ -15,8 +15,8 @@ def initialize(custom_help_message: nil) @custom_help_message = custom_help_message end - sig { override.params(offenses: T::Array[BasicReferenceOffense], repo_link: String, org_name: String).returns(String) } - def format_offenses(offenses, repo_link, org_name) + sig { override.params(offenses: T::Array[BasicReferenceOffense], repo_link: String, org_name: String, modularization_library: String).returns(String) } + def format_offenses(offenses, repo_link, org_name, modularization_library: 'packwerk') violation = T.must(offenses.first) referencing_file_pack = ParsePackwerk.package_from_path(violation.file) # We remove leading double colons as they feel like an implementation detail of packwerk. diff --git a/lib/danger-packwerk/update/offenses_formatter.rb b/lib/danger-packwerk/update/offenses_formatter.rb index 326bd98..36f53fc 100644 --- a/lib/danger-packwerk/update/offenses_formatter.rb +++ b/lib/danger-packwerk/update/offenses_formatter.rb @@ -14,10 +14,11 @@ module OffensesFormatter abstract.params( offenses: T::Array[BasicReferenceOffense], repo_link: String, - org_name: String + org_name: String, + modularization_library: String ).returns(String) end - def format_offenses(offenses, repo_link, org_name); end + def format_offenses(offenses, repo_link, org_name, modularization_library: 'packwerk'); end end end end diff --git a/spec/danger_packwerk/danger_package_todo_yml_changes_spec.rb b/spec/danger_packwerk/danger_package_todo_yml_changes_spec.rb index ec958da..f23981e 100644 --- a/spec/danger_packwerk/danger_package_todo_yml_changes_spec.rb +++ b/spec/danger_packwerk/danger_package_todo_yml_changes_spec.rb @@ -139,6 +139,34 @@ module DangerPackwerk EXPECTED ).and_nothing_else end + + context 'a modularization_library is passed in' do + let!(:modularization_library) { 'pks' } + + it 'displays a markdown with a reasonable message' do + subject + + expect('packs/some_pack/package_todo.yml').to contain_inline_markdown( + <<~EXPECTED + --- + packs/some_other_pack: + "OtherPackClass": + violations: + - privacy + - dependency + files: + - packs/some_pack/some_class.rb + ==================== DANGER_START + Hi again! It looks like `OtherPackClass` is private API of `packs/some_other_pack`, which is also not in `packs/some_pack`'s list of dependencies. + We noticed you ran `bin/packwerk update-todo`. Check out [the docs](https://github.com/Shopify/packwerk/blob/main/RESOLVING_VIOLATIONS.md) to see other ways to resolve violations. + + - Could you add some context as a reply here about why we needed to add these violations? + + ==================== DANGER_END + EXPECTED + ).and_nothing_else + end + end end context 'a offenses formatter is passed in' do @@ -146,7 +174,7 @@ module DangerPackwerk Class.new do include Update::OffensesFormatter - def format_offenses(added_violations, repo_link, org_name) + def format_offenses(added_violations, repo_link, org_name, modularization_library: 'packwerk') <<~MESSAGE There are #{added_violations.count} new violations, with class_names #{added_violations.map(&:class_name).uniq.sort}, @@ -1160,7 +1188,7 @@ def format_offenses(added_violations, repo_link, org_name) Class.new do include Update::OffensesFormatter - def format_offenses(added_violations, repo_link, org_name) + def format_offenses(added_violations, repo_link, org_name, modularization_library: 'packwerk') <<~MESSAGE There are #{added_violations.count} new violations, with class_names #{added_violations.map(&:class_name).uniq.sort}, From 526e7ec988dc3df35473c2ba0c3a975888ac3451 Mon Sep 17 00:00:00 2001 From: Teal Stannard Date: Fri, 31 Jan 2025 13:55:46 -0800 Subject: [PATCH 2/2] change messaging for pks vs packwerk Co-authored-by: Ashley Willard --- .../check/default_formatter.rb | 6 ++- lib/danger-packwerk/danger_packwerk.rb | 13 ++++-- .../update/default_formatter.rb | 6 ++- .../danger_package_todo_yml_changes_spec.rb | 6 ++- spec/danger_packwerk/danger_packwerk_spec.rb | 46 ++++++++++++++++++- 5 files changed, 68 insertions(+), 9 deletions(-) diff --git a/lib/danger-packwerk/check/default_formatter.rb b/lib/danger-packwerk/check/default_formatter.rb index 06838a1..c11ecd0 100644 --- a/lib/danger-packwerk/check/default_formatter.rb +++ b/lib/danger-packwerk/check/default_formatter.rb @@ -39,7 +39,11 @@ def format_offenses(offenses, repo_link, org_name, modularization_library: 'pack constant_source_package = T.must(ParsePackwerk.all.find { |p| p.name == constant_source_package_name }) constant_source_package_ownership_info = Private::OwnershipInformation.for_package(constant_source_package, org_name) - disclaimer = 'Before you run `bin/packwerk update-todo`, check out these quick suggestions:' + if modularization_library == 'packwerk' + disclaimer = 'Before you run `bin/packwerk update-todo`, check out these quick suggestions:' + elsif modularization_library == 'pks' + disclaimer = 'Before you run `bin/pks update`, check out these quick suggestions:' + end referencing_code_in_right_pack = "- Does the code you are writing live in the right pack?\n - If not, try `bin/packs move packs/destination_pack #{referencing_file}`" referenced_code_in_right_pack = "- Does #{constant_name} live in the right pack?\n - If not, try `bin/packs move packs/destination_pack #{constant_location}`" dependency_violation_message = "- Do we actually want to depend on #{constant_source_package_name}?\n - If so, try `bin/packs add_dependency #{referencing_file_pack} #{constant_source_package_name}`\n - If not, what can we change about the design so we do not have to depend on #{constant_source_package_name}?" diff --git a/lib/danger-packwerk/danger_packwerk.rb b/lib/danger-packwerk/danger_packwerk.rb index 8ab97df..9e3aeaf 100644 --- a/lib/danger-packwerk/danger_packwerk.rb +++ b/lib/danger-packwerk/danger_packwerk.rb @@ -45,7 +45,8 @@ class CommentGroupingStrategy < ::T::Enum on_failure: OnFailure, violation_types: T::Array[String], grouping_strategy: CommentGroupingStrategy, - root_path: T.nilable(String) + root_path: T.nilable(String), + modularization_library: String ).void end def check( @@ -56,7 +57,8 @@ def check( on_failure: DEFAULT_ON_FAILURE, violation_types: DEFAULT_VIOLATION_TYPES, grouping_strategy: CommentGroupingStrategy::PerConstantPerLocation, - root_path: nil + root_path: nil, + modularization_library: 'packwerk' ) offenses_formatter ||= Check::DefaultFormatter.new repo_link = github.pr_json[:base][:repo][:html_url] @@ -136,7 +138,12 @@ def check( line_number = reference_offense.location&.line referencing_file = reference_offense.reference.relative_path - message = offenses_formatter.format_offenses(unique_packwerk_reference_offenses, repo_link, org_name) + message = offenses_formatter.format_offenses( + unique_packwerk_reference_offenses, + repo_link, + org_name, + modularization_library: modularization_library + ) markdown(message, file: git_filesystem.convert_to_filesystem(referencing_file), line: line_number) end diff --git a/lib/danger-packwerk/update/default_formatter.rb b/lib/danger-packwerk/update/default_formatter.rb index 804081e..58ffc5c 100644 --- a/lib/danger-packwerk/update/default_formatter.rb +++ b/lib/danger-packwerk/update/default_formatter.rb @@ -28,7 +28,11 @@ def format_offenses(offenses, repo_link, org_name, modularization_library: 'pack package_referring_to_constant_owner = Private::OwnershipInformation.for_package(referencing_file_pack, org_name) - disclaimer = 'We noticed you ran `bin/packwerk update-todo`. Check out [the docs](https://github.com/Shopify/packwerk/blob/main/RESOLVING_VIOLATIONS.md) to see other ways to resolve violations.' + if modularization_library == 'packwerk' + disclaimer = 'We noticed you ran `bin/packwerk update-todo`. Check out [the docs](https://github.com/Shopify/packwerk/blob/main/RESOLVING_VIOLATIONS.md) to see other ways to resolve violations.' + elsif modularization_library == 'pks' + disclaimer = 'We noticed you ran `bin/pks update`. Check out [the docs](https://github.com/Shopify/packwerk/blob/main/RESOLVING_VIOLATIONS.md) to see other ways to resolve violations.' + end pluralized_violation = offenses.count > 1 ? 'these violations' : 'this violation' request_to_add_context = "- Could you add some context as a reply here about why we needed to add #{pluralized_violation}?\n" diff --git a/spec/danger_packwerk/danger_package_todo_yml_changes_spec.rb b/spec/danger_packwerk/danger_package_todo_yml_changes_spec.rb index f23981e..4c05db4 100644 --- a/spec/danger_packwerk/danger_package_todo_yml_changes_spec.rb +++ b/spec/danger_packwerk/danger_package_todo_yml_changes_spec.rb @@ -29,10 +29,12 @@ module DangerPackwerk subject do danger_package_todo_yml_changes.check( before_comment: before_comment, - root_path: root_path + root_path: root_path, + modularization_library: modularization_library ) end + let(:modularization_library) { 'packwerk' } let(:some_pack_package_todo_before) { nil } let(:diff_double) { sorbet_double(Git::Diff::DiffFile) } @@ -158,7 +160,7 @@ module DangerPackwerk - packs/some_pack/some_class.rb ==================== DANGER_START Hi again! It looks like `OtherPackClass` is private API of `packs/some_other_pack`, which is also not in `packs/some_pack`'s list of dependencies. - We noticed you ran `bin/packwerk update-todo`. Check out [the docs](https://github.com/Shopify/packwerk/blob/main/RESOLVING_VIOLATIONS.md) to see other ways to resolve violations. + We noticed you ran `bin/pks update`. Check out [the docs](https://github.com/Shopify/packwerk/blob/main/RESOLVING_VIOLATIONS.md) to see other ways to resolve violations. - Could you add some context as a reply here about why we needed to add these violations? diff --git a/spec/danger_packwerk/danger_packwerk_spec.rb b/spec/danger_packwerk/danger_packwerk_spec.rb index 0e53255..6e98b55 100644 --- a/spec/danger_packwerk/danger_packwerk_spec.rb +++ b/spec/danger_packwerk/danger_packwerk_spec.rb @@ -30,7 +30,7 @@ module DangerPackwerk Class.new do include Check::OffensesFormatter - def format_offenses(offenses, repo_link, org_name) + def format_offenses(offenses, repo_link, org_name, modularization_library:) offenses.map(&:message).join("\n\n") end end @@ -775,7 +775,8 @@ def format_offenses(offenses, repo_link, org_name) grouping_strategy: DangerPackwerk::PerConstantPerPackGrouping, offenses_formatter: Check::DefaultFormatter.new( custom_help_message: 'Need help? Join us in #ruby-modularity or see go/packs.' - ) + ), + modularization_library: modularization_library ) end @@ -816,6 +817,7 @@ def format_offenses(offenses, repo_link, org_name) YML end + let(:modularization_library) { 'packwerk' } let(:danger_packwerk) { dangerfile.packwerk } let(:generic_privacy_violation) do sorbet_double( @@ -889,6 +891,46 @@ def format_offenses(offenses, repo_link, org_name) expect(actual_markdown.type).to eq :markdown end + context 'modularization_library is pks' do + let(:modularization_library) { 'pks' } + + it 'uses the pks update command' do + subject + expect(dangerfile.status_report[:warnings]).to be_empty + expect(dangerfile.status_report[:errors]).to be_empty + actual_markdowns = dangerfile.status_report[:markdowns] + expect(actual_markdowns.count).to eq 1 + actual_markdown = actual_markdowns.first + expected = <<~EXPECTED + **Packwerk Violation** + - Type: Privacy :lock: + - Constant: [`PrivateConstant`](https://github.com/MyOrg/my_repo/blob/main/packs/gusto_slack/app/services/private_constant.rb) + - Owning pack: packs/gusto_slack + - Owned by [@MyOrg/product-infrastructure](https://github.com/orgs/MyOrg/teams/product-infrastructure/members) (Slack: [#prod-infra](https://slack.com/app_redirect?channel=prod-infra)) + +
Quick suggestions :bulb: + + Before you run `bin/pks update`, check out these quick suggestions: + - Does the code you are writing live in the right pack? + - If not, try `bin/packs move packs/destination_pack packs/referencing_package/some_file.rb` + - Does PrivateConstant live in the right pack? + - If not, try `bin/packs move packs/destination_pack packs/gusto_slack/app/services/private_constant.rb` + - Does API in packs/gusto_slack/public support this use case? + - If not, can we work with [@MyOrg/product-infrastructure](https://github.com/orgs/MyOrg/teams/product-infrastructure/members) to create and use a public API? + - If `PrivateConstant` should already be public, try `bin/packs make_public packs/gusto_slack/app/services/private_constant.rb`. + +
+ + _Need help? Join us in #ruby-modularity or see go/packs._ + EXPECTED + + expect(actual_markdown.message).to eq expected + expect(actual_markdown.line).to eq 12 + expect(actual_markdown.file).to eq 'packs/referencing_package/some_file.rb' + expect(actual_markdown.type).to eq :markdown + end + end + context 'there is no owning team' do before do allow(CodeOwnership).to receive(:for_package).and_return(nil)