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

Switch directory ownership to git ls-files #80

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
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
5 changes: 4 additions & 1 deletion lib/code_ownership.rb
Original file line number Diff line number Diff line change
Expand Up @@ -85,14 +85,17 @@ def self.remove_file_annotation!(filename)
autocorrect: T::Boolean,
stage_changes: T::Boolean,
files: T.nilable(T::Array[String]),
use_git_ls_files: T::Boolean,
).void
end
def validate!(
autocorrect: true,
stage_changes: true,
files: nil
files: nil,
use_git_ls_files: false
)
Private.load_configuration!
configuration.use_git_ls_files = use_git_ls_files

tracked_file_subset = if files
files.select{|f| Private.file_tracked?(f)}
Expand Down
8 changes: 7 additions & 1 deletion lib/code_ownership/cli.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@ def self.validate!(argv)
options[:skip_stage] = true
end

opts.on('--use-git-ls-files',
'Use git ls-files for findinging .codeowner files instead of globs. Will be faster if in a git context.') do
options[:use_git_ls_files] = true
end

opts.on('--help', 'Shows this prompt') do
puts opts
exit
Expand All @@ -67,7 +72,8 @@ def self.validate!(argv)
CodeOwnership.validate!(
files: files,
autocorrect: !options[:skip_autocorrect],
stage_changes: !options[:skip_stage]
stage_changes: !options[:skip_stage],
use_git_ls_files: !!options[:use_git_ls_files]
)
end

Expand Down
4 changes: 3 additions & 1 deletion lib/code_ownership/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ class Configuration < T::Struct
const :skip_codeowners_validation, T::Boolean
const :raw_hash, T::Hash[T.untyped, T.untyped]
const :require_github_teams, T::Boolean
prop :use_git_ls_files, T::Boolean

sig { returns(Configuration) }
def self.fetch
Expand All @@ -29,7 +30,8 @@ def self.fetch
js_package_paths: js_package_paths(config_hash),
skip_codeowners_validation: config_hash.fetch('skip_codeowners_validation', false),
raw_hash: config_hash,
require_github_teams: config_hash.fetch('require_github_teams', false)
require_github_teams: config_hash.fetch('require_github_teams', false),
use_git_ls_files: config_hash.fetch('use_git_ls_files', false)
)
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,22 @@ def update_cache(cache, files)
# subset of files, but rather we want code ownership for all files.
#
sig do
override.params(files: T::Array[String]).
returns(T::Hash[String, ::CodeTeams::Team])
override.params(_files: T::Array[String])
.returns(T::Hash[String, ::CodeTeams::Team])
end
def globs_to_owner(files)
# The T.unsafe is because the upstream RBI is wrong for Pathname.glob
T
.unsafe(Pathname)
.glob(File.join('**/', CODEOWNERS_DIRECTORY_FILE_NAME))
.map(&:cleanpath)
.each_with_object({}) do |pathname, res|
def globs_to_owner(_files)
file_list =
if CodeOwnership.configuration.use_git_ls_files
`git ls-files **/#{CODEOWNERS_DIRECTORY_FILE_NAME}`
.each_line
.map { Pathname.new(_1.strip).cleanpath }
else
T
.unsafe(Pathname)
.glob(File.join('**/', CODEOWNERS_DIRECTORY_FILE_NAME))
.map(&:cleanpath)
end
file_list.each_with_object({}) do |pathname, res|
owner = owner_for_codeowners_file(pathname)
res[pathname.dirname.cleanpath.join('**/**').to_s] = owner
end
Expand Down
17 changes: 17 additions & 0 deletions spec/lib/code_ownership_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,23 @@
expect { CodeOwnership.validate!(files: ['app/services/test/some_unowned_file.rb']) }.to_not raise_error
expect { CodeOwnership.validate!(files: ['app/services/[test]/some_unowned_file.rb']) }.to_not raise_error
end

context 'when the the configuration for use_git_ls_files is enabled' do
before do
allow(CodeOwnership.configuration).to receive(:use_git_ls_files) { true }
allow_any_instance_of(Object).to receive(:`) do
Pathname
.glob(File.join('**/', '.codeowner'))
.map(&:to_s)
.join("\n")
end
end

it 'has no validation errors' do
expect { CodeOwnership.validate!(files: ['app/services/test/some_unowned_file.rb']) }.to_not raise_error
expect { CodeOwnership.validate!(files: ['app/services/[test]/some_unowned_file.rb']) }.to_not raise_error
end
end
end

context 'file ownership with [] characters' do
Expand Down