From 739b7c40de17cb9ea63976f452a6958fb9a94a5c Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Sun, 27 Sep 2026 10:36:10 -0700 Subject: [PATCH 1/2] Read and set a package's top-level owner `ParsePackwerk::Package` had no way to read or change the top-level `owner` key, even though `write_package_yml!` already orders it among the canonical keys: `package.owner` raised NoMethodError, and `package.with(owner: "x")` raised ArgumentError, since T::Struct#with only accepts declared props. The only way in was editing `package.config['owner']` by hand. `owner` is a reader over `config['owner']` rather than a new prop, and `with(owner:)` sets that key, or removes it when given nil. With a separate prop, a package loaded from disk would hold its owner in two places, and the prop would silently override any edit made through `config`, which is the workaround in use today. Keeping `config` as the only store leaves every path that works on main byte-identical. A value that isn't a String reads as nil and is still written back unchanged. `metadata.owner` is a separate, older convention and isn't read into `owner`. Fixes #49. Also bumps the version to 0.28.0. --- Gemfile.lock | 2 +- lib/parse_packwerk/constants.rb | 1 + lib/parse_packwerk/package.rb | 22 ++++ parse_packwerk.gemspec | 2 +- spec/parse_packwerk_spec.rb | 155 +++++++++++++++++++++++++- spec/support/have_matching_package.rb | 1 + 6 files changed, 179 insertions(+), 4 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index c81c0a3..5608d57 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - parse_packwerk (0.27.0) + parse_packwerk (0.28.0) bigdecimal sorbet-runtime diff --git a/lib/parse_packwerk/constants.rb b/lib/parse_packwerk/constants.rb index c3f5038..867269f 100644 --- a/lib/parse_packwerk/constants.rb +++ b/lib/parse_packwerk/constants.rb @@ -12,6 +12,7 @@ module ParsePackwerk DEPENDENCY_VIOLATION_TYPE = T.let('dependency', String) PRIVACY_VIOLATION_TYPE = T.let('privacy', String) PUBLIC_PATH = T.let('public_path', String) + OWNER = T.let('owner', String) METADATA = T.let('metadata', String) DEPENDENCIES = T.let('dependencies', String) diff --git a/lib/parse_packwerk/package.rb b/lib/parse_packwerk/package.rb index a97278d..efc7ce8 100644 --- a/lib/parse_packwerk/package.rb +++ b/lib/parse_packwerk/package.rb @@ -72,5 +72,27 @@ def enforces_privacy? def enforces_layers? enforce_layers end + + # `owner` is read from `config` rather than stored as a prop, so edits through `config` and `with(owner:)` can't disagree. + sig { returns(T.nilable(String)) } + def owner + owner = config[OWNER] + owner if owner.is_a?(String) + end + + sig { params(changed_props: T::Hash[Symbol, T.untyped]).returns(Package) } + def with(changed_props) + return super unless changed_props.key?(:owner) + + changes = changed_props.dup + new_owner = changes.delete(:owner) + new_config = T.let(changes.fetch(:config, config), T::Hash[T.untyped, T.untyped]).dup + if new_owner.nil? + new_config.delete(OWNER) + else + new_config[OWNER] = new_owner + end + super(changes.merge(config: new_config)) + end end end diff --git a/parse_packwerk.gemspec b/parse_packwerk.gemspec index 98fa3e5..808de61 100644 --- a/parse_packwerk.gemspec +++ b/parse_packwerk.gemspec @@ -2,7 +2,7 @@ Gem::Specification.new do |spec| spec.name = 'parse_packwerk' - spec.version = '0.27.0' + spec.version = '0.28.0' spec.authors = ['Gusto Engineers'] spec.email = ['dev@gusto.com'] spec.summary = 'A low-dependency gem for parsing and writing packwerk YML files' diff --git a/spec/parse_packwerk_spec.rb b/spec/parse_packwerk_spec.rb index 2280874..3344321 100644 --- a/spec/parse_packwerk_spec.rb +++ b/spec/parse_packwerk_spec.rb @@ -319,6 +319,60 @@ it { is_expected.to have_matching_package expected_domain_package, expected_package_todo } end + context 'in app that has a top-level owner' do + before do + write_file('packs/package1/package.yml', <<~CONTENTS) + enforce_dependencies: true + enforce_privacy: true + owner: Mission > Team + metadata: + owner: Legacy Team + CONTENTS + write_file('packs/package2/package.yml', <<~CONTENTS) + enforce_dependencies: true + CONTENTS + end + + it 'reads the top-level owner, separately from metadata' do + package = ParsePackwerk.find('packs/package1') + + expect(package.owner).to eq 'Mission > Team' + expect(package.metadata).to eq('owner' => 'Legacy Team') + end + + it 'is nil for a package without one' do + expect(ParsePackwerk.find('packs/package2').owner).to be_nil + end + + it 'does not fall back to metadata.owner' do + write_file('packs/package2/package.yml', <<~CONTENTS) + enforce_dependencies: true + metadata: + owner: Legacy Team + CONTENTS + + expect(ParsePackwerk.find('packs/package2').owner).to be_nil + end + + context 'when the owner is not a string' do + before do + write_file('packs/package1/package.yml', <<~CONTENTS) + enforce_dependencies: true + owner: 123 + CONTENTS + end + + it 'still loads the package, with a nil owner, and writes the value back unchanged' do + package = ParsePackwerk.find('packs/package1') + expect(package.owner).to be_nil + + ParsePackwerk.write_package_yml!(package) + + expect(YAML.load_file('packs/package1/package.yml')['owner']).to eq 123 + end + end + end + context 'in app that has violations' do before do write_file('packs/package2/package_todo.yml', <<~CONTENTS) @@ -983,7 +1037,7 @@ let(:package_yml) { package_dir.join('package.yml') } let(:package_todo_yml) { package_dir.join('package_todo.yml') } - def build_pack(public_path: 'app/public', enforce_privacy: true, enforce_layers: true, dependencies: [], metadata: {}, config: {}) + def build_pack(public_path: 'app/public', enforce_privacy: true, enforce_layers: true, owner: nil, dependencies: [], metadata: {}, config: {}) ParsePackwerk::Package.new( name: package_dir.to_s, enforce_dependencies: true, @@ -992,7 +1046,7 @@ def build_pack(public_path: 'app/public', enforce_privacy: true, enforce_layers: public_path: public_path, dependencies: dependencies, metadata: metadata, - config: config, + config: owner ? config.merge('owner' => owner) : config, violations: [] ) end @@ -1003,6 +1057,7 @@ def pack_as_hash(package) enforce_dependencies: package.enforce_dependencies, enforce_privacy: package.enforce_privacy, enforce_layers: package.enforce_layers, + owner: package.owner, dependencies: package.dependencies, metadata: package.metadata } @@ -1159,6 +1214,86 @@ def pack_as_hash(package) end end + context 'package with owner' do + let(:package) { build_pack(owner: 'Mission > Team', dependencies: ['packs/foo']) } + + it 'writes owner as a top-level key' do + ParsePackwerk.write_package_yml!(package) + + expect(package_yml.read).to eq <<~PACKAGEYML + enforce_dependencies: true + enforce_privacy: true + enforce_layers: true + owner: Mission > Team + dependencies: + - packs/foo + PACKAGEYML + + expect(all_packages.count).to eq 1 + expect(pack_as_hash(all_packages.first)).to eq pack_as_hash(package) + end + + context 'overwriting an existing package file' do + before do + write_file(package_yml, <<~CONTENTS) + enforce_dependencies: true + enforce_privacy: true + enforce_layers: true + owner: Old Team + dependencies: + - packs/foo + CONTENTS + end + + it 'allows you to change the owner' do + new_package = ParsePackwerk.find('packs/example_pack').with(owner: 'New Team') + + ParsePackwerk.write_package_yml!(new_package) + + expect(package_yml.read).to eq <<~PACKAGEYML + enforce_dependencies: true + enforce_privacy: true + enforce_layers: true + owner: New Team + dependencies: + - packs/foo + PACKAGEYML + end + + it 'writes the file back unchanged when the owner is untouched' do + original = package_yml.read + + ParsePackwerk.write_package_yml!(ParsePackwerk.find('packs/example_pack')) + + expect(package_yml.read).to eq original + end + + it 'allows you to remove the owner' do + new_package = ParsePackwerk.find('packs/example_pack').with(owner: nil) + + ParsePackwerk.write_package_yml!(new_package) + + expect(package_yml.read).to eq <<~PACKAGEYML + enforce_dependencies: true + enforce_privacy: true + enforce_layers: true + dependencies: + - packs/foo + PACKAGEYML + end + + it 'still honours an owner changed directly in config' do + package = ParsePackwerk.find('packs/example_pack') + package.config['owner'] = 'New Team' + + ParsePackwerk.write_package_yml!(package) + + expect(package.owner).to eq 'New Team' + expect(YAML.load_file(package_yml)['owner']).to eq 'New Team' + end + end + end + context 'package with other top-level config' do let(:package) do build_pack(config: { @@ -1280,6 +1415,22 @@ def pack_as_hash(package) PACKAGEYML end + it 'appends a newly added owner as a new key' do + package = ParsePackwerk::Package.from(package_yml) + new_package = package.with(owner: 'Team A') + ParsePackwerk.write_package_yml!(new_package) + + expect(package_yml.read).to eq <<~PACKAGEYML + enforce_privacy: true + enforce_layers: true + layer: admin + enforce_dependencies: true + dependencies: + - packs/foo + owner: Team A + PACKAGEYML + end + it 'handles removed keys gracefully' do package = ParsePackwerk::Package.from(package_yml) # Remove dependencies diff --git a/spec/support/have_matching_package.rb b/spec/support/have_matching_package.rb index 1a4950b..13508a8 100644 --- a/spec/support/have_matching_package.rb +++ b/spec/support/have_matching_package.rb @@ -33,6 +33,7 @@ def deep_hashify_package(package, package_todo) name: package.name, enforce_dependencies: package.enforce_dependencies, enforce_privacy: package.enforce_privacy, + owner: package.owner, metadata: package.metadata, dependencies: package.dependencies.sort, package_todo: if package_todo.nil? From 052dce79f0bc8da21bf35c85bb6adbec13ae9a33 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Sun, 27 Sep 2026 11:07:08 -0700 Subject: [PATCH 2/2] Keep the rest of package.yml intact when setting owner `with(owner:)` passed the whole config back through `T::Struct#with`, which stringifies every hash key it is given, so a nested key such as `1:` or `true:` was written back as `'1':` or `'true':`. It now edits the copy that `super` returns, which is already a deep copy of the receiver's config, so only the owner line changes and the file matches what the `config['owner'] = ...` workaround writes. This also stops `with(owner:, 'config' => {...})` from discarding the given config. `with(owner:)` now raises TypeError for a value that isn't a String or nil, rather than writing one that `owner` would then read as nil. The comment on `owner` notes that it reads only the top-level key, unlike `CodeOwnership.for_package`, which falls back to `metadata.owner`. --- lib/parse_packwerk/package.rb | 16 +++++++++------- spec/parse_packwerk_spec.rb | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/lib/parse_packwerk/package.rb b/lib/parse_packwerk/package.rb index efc7ce8..0e14f95 100644 --- a/lib/parse_packwerk/package.rb +++ b/lib/parse_packwerk/package.rb @@ -74,6 +74,7 @@ def enforces_layers? end # `owner` is read from `config` rather than stored as a prop, so edits through `config` and `with(owner:)` can't disagree. + # Only the top-level key is read. Unlike `CodeOwnership.for_package`, this doesn't fall back to `metadata.owner`. sig { returns(T.nilable(String)) } def owner owner = config[OWNER] @@ -85,14 +86,15 @@ def with(changed_props) return super unless changed_props.key?(:owner) changes = changed_props.dup - new_owner = changes.delete(:owner) - new_config = T.let(changes.fetch(:config, config), T::Hash[T.untyped, T.untyped]).dup - if new_owner.nil? - new_config.delete(OWNER) - else - new_config[OWNER] = new_owner + new_owner = T.let(changes.delete(:owner), T.nilable(String)) + # Edit the copy `super` returns rather than passing `config:` to it, which would stringify every nested key in the file. + super(changes).tap do |package| + if new_owner.nil? + package.config.delete(OWNER) + else + package.config[OWNER] = new_owner + end end - super(changes.merge(config: new_config)) end end end diff --git a/spec/parse_packwerk_spec.rb b/spec/parse_packwerk_spec.rb index 3344321..f2b5536 100644 --- a/spec/parse_packwerk_spec.rb +++ b/spec/parse_packwerk_spec.rb @@ -1268,6 +1268,38 @@ def pack_as_hash(package) expect(package_yml.read).to eq original end + it 'changes only the owner line when other config has non-string keys' do + write_file(package_yml, <<~CONTENTS) + enforce_dependencies: true + enforce_privacy: true + enforce_layers: true + owner: Old Team + dependencies: + - packs/foo + custom: + 1: one + true: enabled + CONTENTS + original = package_yml.read + + ParsePackwerk.write_package_yml!(ParsePackwerk.find('packs/example_pack').with(owner: 'New Team')) + + expect(package_yml.read).to eq original.sub('owner: Old Team', 'owner: New Team') + end + + it 'leaves the package it was called on unchanged' do + package = ParsePackwerk.find('packs/example_pack') + + package.with(owner: 'New Team') + + expect(package.owner).to eq 'Old Team' + expect(ParsePackwerk.find('packs/example_pack').owner).to eq 'Old Team' + end + + it 'rejects an owner that is not a string' do + expect { ParsePackwerk.find('packs/example_pack').with(owner: 123) }.to raise_error(TypeError) + end + it 'allows you to remove the owner' do new_package = ParsePackwerk.find('packs/example_pack').with(owner: nil)