From 88f25ff627a3504a7e5b6190e89148ea8f1ec9cc Mon Sep 17 00:00:00 2001 From: Alex Evanczuk Date: Mon, 24 Oct 2022 15:04:04 -0400 Subject: [PATCH 1/3] # This is a combination of 2 commits. # This is the 1st commit message: Remove coupling to PackageProtections # The commit message #2 will be skipped: # wip --- Gemfile | 2 +- Gemfile.lock | 49 ++++++++++++------- .../private/metrics/protection_usage.rb | 15 +++++- .../metrics/rubocop_protections_exclusions.rb | 2 + spec/modularization_statistics_spec.rb | 6 --- 5 files changed, 48 insertions(+), 26 deletions(-) diff --git a/Gemfile b/Gemfile index 67e8777..c579e6b 100644 --- a/Gemfile +++ b/Gemfile @@ -4,5 +4,5 @@ source 'https://rubygems.org' gemspec gem 'code_ownership' -gem 'package_protections' +gem 'package_protections', path: '../package_protections' gem 'parse_packwerk' diff --git a/Gemfile.lock b/Gemfile.lock index 61a2155..539c3dd 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,3 +1,14 @@ +PATH + remote: ../package_protections + specs: + package_protections (2.5.2) + activesupport + parse_packwerk + rubocop + rubocop-packs + rubocop-sorbet + sorbet-runtime + PATH remote: . specs: @@ -12,7 +23,7 @@ PATH GEM remote: https://rubygems.org/ specs: - activesupport (7.0.2.4) + activesupport (7.0.4) concurrent-ruby (~> 1.0, >= 1.0.2) i18n (>= 1.6, < 2) minitest (>= 5.1) @@ -29,17 +40,12 @@ GEM diff-lcs (1.5.0) dogapi (1.45.0) multi_json - i18n (1.10.0) + i18n (1.12.0) concurrent-ruby (~> 1.0) + json (2.6.2) method_source (1.0.0) - minitest (5.15.0) + minitest (5.16.3) multi_json (1.15.0) - package_protections (1.4.0) - activesupport - parse_packwerk - rubocop - rubocop-sorbet - sorbet-runtime parallel (1.22.1) parse_packwerk (0.14.0) sorbet-runtime @@ -55,7 +61,7 @@ GEM parser (>= 2.6.4.0) sorbet-runtime (>= 0.5.9204) unparser - regexp_parser (2.2.1) + regexp_parser (2.6.0) rexml (3.2.5) rspec (3.11.0) rspec-core (~> 3.11.0) @@ -70,18 +76,25 @@ GEM diff-lcs (>= 1.2.0, < 2.0) rspec-support (~> 3.11.0) rspec-support (3.11.0) - rubocop (1.26.1) + rubocop (1.33.0) + json (~> 2.3) parallel (~> 1.10) parser (>= 3.1.0.0) rainbow (>= 2.2.2, < 4.0) regexp_parser (>= 1.8, < 3.0) - rexml - rubocop-ast (>= 1.16.0, < 2.0) + rexml (>= 3.2.5, < 4.0) + rubocop-ast (>= 1.19.1, < 2.0) ruby-progressbar (~> 1.7) unicode-display_width (>= 1.4.0, < 3.0) - rubocop-ast (1.16.0) + rubocop-ast (1.23.0) parser (>= 3.1.1.0) - rubocop-sorbet (0.6.7) + rubocop-packs (0.0.13) + activesupport + parse_packwerk + rubocop + rubocop-sorbet + sorbet-runtime + rubocop-sorbet (0.6.11) rubocop (>= 0.90.0) ruby-progressbar (1.11.0) sorbet (0.5.9924) @@ -110,9 +123,9 @@ GEM thor (>= 1.2.0) yard-sorbet thor (1.2.1) - tzinfo (2.0.4) + tzinfo (2.0.5) concurrent-ruby (~> 1.0) - unicode-display_width (2.1.0) + unicode-display_width (2.3.0) unparser (0.6.4) diff-lcs (~> 1.3) parser (>= 3.1.0) @@ -132,7 +145,7 @@ PLATFORMS DEPENDENCIES code_ownership modularization_statistics! - package_protections + package_protections! parse_packwerk pry rake diff --git a/lib/modularization_statistics/private/metrics/protection_usage.rb b/lib/modularization_statistics/private/metrics/protection_usage.rb index 80ae1a8..35ed961 100644 --- a/lib/modularization_statistics/private/metrics/protection_usage.rb +++ b/lib/modularization_statistics/private/metrics/protection_usage.rb @@ -9,8 +9,21 @@ class ProtectionUsage sig { params(prefix: String, packages: T::Array[ParsePackwerk::Package], package_tags: T::Array[Tag]).returns(T::Array[GaugeMetric]) } def self.get_protections_metrics(prefix, packages, package_tags) + # These should look at native packwerk...? + # Perhaps two implementations: + # If the package has a protections key, use the old implementation. + # If it doesn't, use the "new" implementation, which checks `enforce_privacy` and `enforce_dependencies`, `.pack_rubocop.yml`, + # `metadata.enforce_privacy_strictly: true, metadata.enforce_dependencies_strictly: true` protected_packages = packages.map { |p| PackageProtections::ProtectedPackage.from(p) } - + # [ + # 'prevent_this_package_from_violating_its_stated_dependencies', + # 'prevent_other_packages_from_using_this_packages_internals', + # 'prevent_this_package_from_exposing_an_untyped_api', + # 'prevent_this_package_from_creating_other_namespaces', + # 'prevent_other_packages_from_using_this_package_without_explicit_visibility', + # 'prevent_this_package_from_exposing_instance_method_public_apis', + # 'prevent_this_package_from_exposing_undocumented_public_apis' + # ] PackageProtections.all.flat_map do |protection| PackageProtections::ViolationBehavior.each_value.map do |violation_behavior| # https://github.com/Gusto/package_protections/pull/42 changed the public API of these violation behaviors. diff --git a/lib/modularization_statistics/private/metrics/rubocop_protections_exclusions.rb b/lib/modularization_statistics/private/metrics/rubocop_protections_exclusions.rb index ddb582e..f1b850e 100644 --- a/lib/modularization_statistics/private/metrics/rubocop_protections_exclusions.rb +++ b/lib/modularization_statistics/private/metrics/rubocop_protections_exclusions.rb @@ -4,6 +4,8 @@ module ModularizationStatistics module Private module Metrics + # TODO: + # Should we create API in `rubocop-packs` for some of this? class RubocopProtectionsExclusions extend T::Sig diff --git a/spec/modularization_statistics_spec.rb b/spec/modularization_statistics_spec.rb index 8369c4b..621674d 100644 --- a/spec/modularization_statistics_spec.rb +++ b/spec/modularization_statistics_spec.rb @@ -107,12 +107,6 @@ module ModularizationStatistics # rubocop:disable RSpec/DescribedClassModuleWrap write_file('packs/only_package/package.yml', <<~CONTENTS) enforce_dependencies: false enforce_privacy: false - metadata: - protections: - prevent_this_package_from_violating_its_stated_dependencies: fail_never - prevent_other_packages_from_using_this_packages_internals: fail_never - prevent_this_package_from_exposing_an_untyped_api: fail_never - prevent_this_package_from_creating_other_namespaces: fail_never CONTENTS write_file('packs/only_package/spec/some_package_file_spec.rb') From 349ce2ccd741cfad7540df86bdac75bdcb91c3aa Mon Sep 17 00:00:00 2001 From: Alex Evanczuk Date: Mon, 24 Oct 2022 16:25:10 -0400 Subject: [PATCH 2/3] Remove coupling to PackageProtections --- Gemfile | 2 +- Gemfile.lock | 20 +++--- .../private/metrics/protection_usage.rb | 56 ++++++++++++----- .../metrics/rubocop_protections_exclusions.rb | 3 +- spec/modularization_statistics_spec.rb | 63 +++++++++++++++++++ 5 files changed, 114 insertions(+), 30 deletions(-) diff --git a/Gemfile b/Gemfile index c579e6b..67e8777 100644 --- a/Gemfile +++ b/Gemfile @@ -4,5 +4,5 @@ source 'https://rubygems.org' gemspec gem 'code_ownership' -gem 'package_protections', path: '../package_protections' +gem 'package_protections' gem 'parse_packwerk' diff --git a/Gemfile.lock b/Gemfile.lock index 539c3dd..1ebce07 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,14 +1,3 @@ -PATH - remote: ../package_protections - specs: - package_protections (2.5.2) - activesupport - parse_packwerk - rubocop - rubocop-packs - rubocop-sorbet - sorbet-runtime - PATH remote: . specs: @@ -46,6 +35,13 @@ GEM method_source (1.0.0) minitest (5.16.3) multi_json (1.15.0) + package_protections (3.0.0) + activesupport + parse_packwerk + rubocop + rubocop-packs + rubocop-sorbet + sorbet-runtime parallel (1.22.1) parse_packwerk (0.14.0) sorbet-runtime @@ -145,7 +141,7 @@ PLATFORMS DEPENDENCIES code_ownership modularization_statistics! - package_protections! + package_protections parse_packwerk pry rake diff --git a/lib/modularization_statistics/private/metrics/protection_usage.rb b/lib/modularization_statistics/private/metrics/protection_usage.rb index 35ed961..366f31b 100644 --- a/lib/modularization_statistics/private/metrics/protection_usage.rb +++ b/lib/modularization_statistics/private/metrics/protection_usage.rb @@ -9,21 +9,7 @@ class ProtectionUsage sig { params(prefix: String, packages: T::Array[ParsePackwerk::Package], package_tags: T::Array[Tag]).returns(T::Array[GaugeMetric]) } def self.get_protections_metrics(prefix, packages, package_tags) - # These should look at native packwerk...? - # Perhaps two implementations: - # If the package has a protections key, use the old implementation. - # If it doesn't, use the "new" implementation, which checks `enforce_privacy` and `enforce_dependencies`, `.pack_rubocop.yml`, - # `metadata.enforce_privacy_strictly: true, metadata.enforce_dependencies_strictly: true` protected_packages = packages.map { |p| PackageProtections::ProtectedPackage.from(p) } - # [ - # 'prevent_this_package_from_violating_its_stated_dependencies', - # 'prevent_other_packages_from_using_this_packages_internals', - # 'prevent_this_package_from_exposing_an_untyped_api', - # 'prevent_this_package_from_creating_other_namespaces', - # 'prevent_other_packages_from_using_this_package_without_explicit_visibility', - # 'prevent_this_package_from_exposing_instance_method_public_apis', - # 'prevent_this_package_from_exposing_undocumented_public_apis' - # ] PackageProtections.all.flat_map do |protection| PackageProtections::ViolationBehavior.each_value.map do |violation_behavior| # https://github.com/Gusto/package_protections/pull/42 changed the public API of these violation behaviors. @@ -37,7 +23,47 @@ def self.get_protections_metrics(prefix, packages, package_tags) } violation_behavior_name = violation_behavior_map[violation_behavior] metric_name = "#{prefix}.#{protection.identifier}.#{violation_behavior_name}.count" - count_of_packages = protected_packages.count { |p| p.violation_behavior_for(protection.identifier) == violation_behavior } + count_of_packages = protected_packages.count do |p| + # + # This is temporarily in place until we migrate off of `package_protections` in favor of `rubocop-packs`. + # At that point, we want to delete this branch and instead it we'd probably have two separate branches. + # One branch would look at `enforce_x` and `metadata.strictly_enforce_x`. + # The other branch would look at `.pack_rubocop.yml`. + # Later on, we could generalize this so that it automatically incorporates new cops from `rubocop-packs`, + # or even new packwerk plugins. + # + # Regardless, we'll want to keep the way we are naming these behaviors for now to preserve historical trends in the data. + # + if p.metadata['protections'] + p.violation_behavior_for(protection.identifier) == violation_behavior + else + case violation_behavior + when PackageProtections::ViolationBehavior::FailOnAny + # There is not yet an implementation for `FailOnAny` for systems that don't use package protections + false + when PackageProtections::ViolationBehavior::FailNever + if protection.identifier == 'prevent_this_package_from_violating_its_stated_dependencies' + !p.original_package.enforces_dependencies? + elsif protection.identifier == 'prevent_other_packages_from_using_this_packages_internals' + !p.original_package.enforces_privacy? + else + # This is not applicable if you're not using package protections + true + end + when PackageProtections::ViolationBehavior::FailOnNew + if protection.identifier == 'prevent_this_package_from_violating_its_stated_dependencies' + p.original_package.enforces_dependencies? + elsif protection.identifier == 'prevent_other_packages_from_using_this_packages_internals' + p.original_package.enforces_privacy? + else + # This is not applicable if you're not using package protections + false + end + else + T.absurd(violation_behavior) + end + end + end GaugeMetric.for(metric_name, count_of_packages, package_tags) end end diff --git a/lib/modularization_statistics/private/metrics/rubocop_protections_exclusions.rb b/lib/modularization_statistics/private/metrics/rubocop_protections_exclusions.rb index f1b850e..18a9ecc 100644 --- a/lib/modularization_statistics/private/metrics/rubocop_protections_exclusions.rb +++ b/lib/modularization_statistics/private/metrics/rubocop_protections_exclusions.rb @@ -4,8 +4,6 @@ module ModularizationStatistics module Private module Metrics - # TODO: - # Should we create API in `rubocop-packs` for some of this? class RubocopProtectionsExclusions extend T::Sig @@ -21,6 +19,7 @@ def self.get_rubocop_exclusions(prefix, packages, package_tags) end end + # TODO: `rubocop-packs` may want to expose API for this sig { params(package: ParsePackwerk::Package, protection: PackageProtections::RubocopProtectionInterface).returns(Integer) } def self.exclude_count_for_package_and_protection(package, protection) rubocop_todo = package.directory.join('.rubocop_todo.yml') diff --git a/spec/modularization_statistics_spec.rb b/spec/modularization_statistics_spec.rb index 621674d..fc86367 100644 --- a/spec/modularization_statistics_spec.rb +++ b/spec/modularization_statistics_spec.rb @@ -101,6 +101,69 @@ module ModularizationStatistics # rubocop:disable RSpec/DescribedClassModuleWrap context 'in app with a simple package owned by one team' do include_context 'only one team' + before do + write_file('empty_file.rb') + write_file('packs/only_package/app/some_package_file.rb') + write_file('packs/only_package/package.yml', <<~CONTENTS) + enforce_dependencies: false + enforce_privacy: false + metadata: + protections: + prevent_this_package_from_violating_its_stated_dependencies: fail_never + prevent_other_packages_from_using_this_packages_internals: fail_never + prevent_this_package_from_exposing_an_untyped_api: fail_never + prevent_this_package_from_creating_other_namespaces: fail_never + CONTENTS + + write_file('packs/only_package/spec/some_package_file_spec.rb') + end + + it 'emits the right metrics' do + expect(metrics).to include_metric GaugeMetric.for('component_files.by_team', 0, Tags.for(['team:Some team', 'app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('packaged_files.by_team', 2, Tags.for(['team:Some team', 'app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('all_files.by_team', 3, Tags.for(['team:Some team', 'app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('component_files.totals', 0, Tags.for(['app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('packaged_files.totals', 2, Tags.for(['app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('all_files.totals', 3, Tags.for(['app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('all_packages.count', 1, Tags.for(['app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('all_packages.dependencies.count', 0, Tags.for(['app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('all_packages.dependency_violations.count', 0, Tags.for(['app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('all_packages.privacy_violations.count', 0, Tags.for(['app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('all_packages.enforcing_dependencies.count', 0, Tags.for(['app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('all_packages.enforcing_privacy.count', 0, Tags.for(['app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('all_packages.with_violations.count', 0, Tags.for(['app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('all_packages.prevent_this_package_from_violating_its_stated_dependencies.fail_the_build_on_any_instances.count', 0, Tags.for(['app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('all_packages.prevent_this_package_from_violating_its_stated_dependencies.fail_the_build_if_new_instances_appear.count', 0, Tags.for(['app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('all_packages.prevent_other_packages_from_using_this_packages_internals.fail_the_build_on_any_instances.count', 0, Tags.for(['app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('all_packages.prevent_other_packages_from_using_this_packages_internals.fail_the_build_if_new_instances_appear.count', 0, Tags.for(['app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('all_packages.prevent_this_package_from_exposing_an_untyped_api.fail_the_build_on_any_instances.count', 0, Tags.for(['app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('all_packages.prevent_this_package_from_exposing_an_untyped_api.fail_the_build_if_new_instances_appear.count', 0, Tags.for(['app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('all_packages.prevent_this_package_from_creating_other_namespaces.fail_the_build_on_any_instances.count', 0, Tags.for(['app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('all_packages.package_based_file_ownership.count', 0, Tags.for(['app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('all_packages.using_public_directory.count', 0, Tags.for(['app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('by_package.dependency_violations.count', 0, Tags.for(['package:packs/only_package', 'app:MyApp', 'team:Unknown'])) + expect(metrics).to include_metric GaugeMetric.for('by_package.privacy_violations.count', 0, Tags.for(['package:packs/only_package', 'app:MyApp', 'team:Unknown'])) + expect(metrics).to include_metric GaugeMetric.for('by_package.outbound_dependency_violations.count', 0, Tags.for(['package:packs/only_package', 'app:MyApp', 'team:Unknown'])) + expect(metrics).to include_metric GaugeMetric.for('by_package.inbound_dependency_violations.count', 0, Tags.for(['package:packs/only_package', 'app:MyApp', 'team:Unknown'])) + expect(metrics).to include_metric GaugeMetric.for('by_package.outbound_privacy_violations.count', 0, Tags.for(['package:packs/only_package', 'app:MyApp', 'team:Unknown'])) + expect(metrics).to include_metric GaugeMetric.for('by_package.inbound_privacy_violations.count', 0, Tags.for(['package:packs/only_package', 'app:MyApp', 'team:Unknown'])) + expect(metrics).to include_metric GaugeMetric.for('by_package.outbound_explicit_dependencies.count', 0, Tags.for(['package:packs/only_package', 'app:MyApp', 'team:Unknown'])) + expect(metrics).to include_metric GaugeMetric.for('by_package.inbound_explicit_dependencies.count', 0, Tags.for(['package:packs/only_package', 'app:MyApp', 'team:Unknown'])) + expect(metrics).to include_metric GaugeMetric.for('all_packages.using_public_directory.count', 0, Tags.for(['app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('all_packages.all_files.count', 2, Tags.for(['app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('all_packages.public_files.count', 0, Tags.for(['app:MyApp'])) + expect(metrics).to include_metric GaugeMetric.for('by_team.using_public_directory.count', 0, Tags.for(['app:MyApp', 'team:Unknown'])) + expect(metrics).to include_metric GaugeMetric.for('by_team.all_files.count', 2, Tags.for(['app:MyApp', 'team:Unknown'])) + expect(metrics).to include_metric GaugeMetric.for('by_team.public_files.count', 0, Tags.for(['app:MyApp', 'team:Unknown'])) + expect(metrics).to include_metric GaugeMetric.for('by_package.using_public_directory.count', 0, Tags.for(['package:packs/only_package', 'app:MyApp', 'team:Unknown'])) + expect(metrics).to include_metric GaugeMetric.for('by_package.all_files.count', 2, Tags.for(['package:packs/only_package', 'app:MyApp', 'team:Unknown'])) + expect(metrics).to include_metric GaugeMetric.for('by_package.public_files.count', 0, Tags.for(['package:packs/only_package', 'app:MyApp', 'team:Unknown'])) + end + end + + context 'in app that does not use package protectiosn with a simple package owned by one team' do + include_context 'only one team' + before do write_file('empty_file.rb') write_file('packs/only_package/app/some_package_file.rb') From 5bb9ea3f2bdfa86e3a129b482c932d7ee134128c Mon Sep 17 00:00:00 2001 From: Alex Evanczuk Date: Mon, 24 Oct 2022 16:41:48 -0400 Subject: [PATCH 3/3] Bump version --- Gemfile.lock | 2 +- modularization_statistics.gemspec | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 1ebce07..752f754 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - modularization_statistics (1.37.0) + modularization_statistics (1.38.0) code_ownership code_teams dogapi diff --git a/modularization_statistics.gemspec b/modularization_statistics.gemspec index cb04838..19948f3 100644 --- a/modularization_statistics.gemspec +++ b/modularization_statistics.gemspec @@ -1,6 +1,6 @@ Gem::Specification.new do |spec| spec.name = 'modularization_statistics' - spec.version = '1.37.0' + spec.version = '1.38.0' spec.authors = ['Gusto Engineers'] spec.email = ['dev@gusto.com']