From fe2469d0ffbfa8f40d6dcef37dcd464c90bf9409 Mon Sep 17 00:00:00 2001 From: Andrey Marchenko Date: Fri, 5 Jan 2024 14:31:53 +0100 Subject: [PATCH 01/13] first version of CODEOWNERS matcher --- lib/datadog/ci/codeowners/matcher.rb | 59 ++++++++++++++++ lib/datadog/ci/codeowners/rule.rb | 18 +++++ sig/datadog/ci/codeowners/matcher.rbs | 21 ++++++ sig/datadog/ci/codeowners/rule.rbs | 19 +++++ spec/datadog/ci/codeowners/matcher_spec.rb | 82 ++++++++++++++++++++++ 5 files changed, 199 insertions(+) create mode 100644 lib/datadog/ci/codeowners/matcher.rb create mode 100644 lib/datadog/ci/codeowners/rule.rb create mode 100644 sig/datadog/ci/codeowners/matcher.rbs create mode 100644 sig/datadog/ci/codeowners/rule.rbs create mode 100644 spec/datadog/ci/codeowners/matcher_spec.rb diff --git a/lib/datadog/ci/codeowners/matcher.rb b/lib/datadog/ci/codeowners/matcher.rb new file mode 100644 index 00000000..844fe89a --- /dev/null +++ b/lib/datadog/ci/codeowners/matcher.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +require_relative "rule" + +module Datadog + module CI + module Codeowners + # Responsible for matching a file path to a list of owners + class Matcher + def initialize(codeowners_file_path) + @rules = parse(codeowners_file_path) + @rules.reverse! + end + + def list_owners(file_path) + @rules.each do |rule| + return rule.owners if rule.match?(file_path) + end + + [] + end + + private + + def parse(file_path) + return [] unless File.exist?(file_path) + + result = [] + + File.open(file_path, "r") do |f| + f.each_line do |line| + line.strip! + + # Skip comments, empty lines, and section lines + next if line.empty? + next if comment?(line) + next if section?(line) + + pattern, *line_owners = line.strip.split(/\s+/) + next if pattern.nil? || pattern.empty? || line_owners.empty? + + result << Rule.new(pattern, line_owners) + end + end + + result + end + + def comment?(line) + line.start_with?("#") + end + + def section?(line) + line.start_with?("[", "^[") && line.end_with?("]") + end + end + end + end +end diff --git a/lib/datadog/ci/codeowners/rule.rb b/lib/datadog/ci/codeowners/rule.rb new file mode 100644 index 00000000..c2f562eb --- /dev/null +++ b/lib/datadog/ci/codeowners/rule.rb @@ -0,0 +1,18 @@ +module Datadog + module CI + module Codeowners + class Rule + attr_reader :pattern, :owners + + def initialize(pattern, owners) + @pattern = pattern + @owners = owners + end + + def match?(file_path) + File.fnmatch?(pattern, file_path) + end + end + end + end +end diff --git a/sig/datadog/ci/codeowners/matcher.rbs b/sig/datadog/ci/codeowners/matcher.rbs new file mode 100644 index 00000000..1bc1925f --- /dev/null +++ b/sig/datadog/ci/codeowners/matcher.rbs @@ -0,0 +1,21 @@ +module Datadog + module CI + module Codeowners + class Matcher + @rules: Array[Rule] + + def initialize: (String codeowners_file_path) -> void + + def list_owners: (String file_path) -> Array[String] + + private + + def parse: (String file_path) -> Array[Rule] + + def comment?: (String line) -> bool + + def section?: (String line) -> bool + end + end + end +end diff --git a/sig/datadog/ci/codeowners/rule.rbs b/sig/datadog/ci/codeowners/rule.rbs new file mode 100644 index 00000000..618e2dc6 --- /dev/null +++ b/sig/datadog/ci/codeowners/rule.rbs @@ -0,0 +1,19 @@ +module Datadog + module CI + module Codeowners + class Rule + @pattern: String + + @owners: Array[String] + + attr_reader pattern: String + + attr_reader owners: Array[String] + + def initialize: (String pattern, Array[String] owners) -> void + + def match?: (String file_path) -> untyped + end + end + end +end diff --git a/spec/datadog/ci/codeowners/matcher_spec.rb b/spec/datadog/ci/codeowners/matcher_spec.rb new file mode 100644 index 00000000..e2de09c9 --- /dev/null +++ b/spec/datadog/ci/codeowners/matcher_spec.rb @@ -0,0 +1,82 @@ +require "datadog/ci/codeowners/matcher" + +RSpec.describe Datadog::CI::Codeowners::Matcher do + let(:codeowners_file_path) { "/path/to/codeowners" } + let(:matcher) { described_class.new(codeowners_file_path) } + + before do + allow(File).to receive(:exist?).with(codeowners_file_path).and_return(true) + allow(File).to receive(:open).with(codeowners_file_path, "r").and_yield(StringIO.new(codeowners_content)) + end + + describe "#list_owners" do + context "when the codeowners file is empty" do + let(:codeowners_content) { "" } + + it "returns an empty array" do + expect(matcher.list_owners("file.rb")).to eq([]) + end + end + + context "when the codeowners file contains matching patterns" do + let(:codeowners_content) do + <<-CODEOWNERS + # Comment line + /path/to/*.rb @owner3 + /path/to/file.rb @owner1 @owner2 + CODEOWNERS + end + + it "returns the list of owners for the matching pattern" do + expect(matcher.list_owners("/path/to/file.rb")).to eq(["@owner1", "@owner2"]) + expect(matcher.list_owners("/path/to/another_file.rb")).to eq(["@owner3"]) + end + end + + context "when the codeowners file contains non-matching patterns" do + let(:codeowners_content) do + <<-CODEOWNERS + /path/to/file.rb @owner1 + CODEOWNERS + end + + it "returns an empty array" do + expect(matcher.list_owners("/path/to/another_file.rb")).to eq([]) + end + end + + context "when the codeowners file contains comments and empty lines" do + let(:codeowners_content) do + <<-CODEOWNERS + # Comment line + /path/to/*.rb @owner2 + + # Another comment line + /path/to/file.rb @owner1 + CODEOWNERS + end + + it "returns the list of owners for the matching pattern" do + expect(matcher.list_owners("/path/to/file.rb")).to eq(["@owner1"]) + expect(matcher.list_owners("/path/to/another_file.rb")).to eq(["@owner2"]) + end + end + + context "when the codeowners file contains section lines" do + let(:codeowners_content) do + <<-CODEOWNERS + [section1] + /path/to/*.rb @owner2 + + [section2] + /path/to/file.rb @owner1 + CODEOWNERS + end + + it "returns the list of owners for the matching pattern" do + expect(matcher.list_owners("/path/to/file.rb")).to eq(["@owner1"]) + expect(matcher.list_owners("/path/to/another_file.rb")).to eq(["@owner2"]) + end + end + end +end From 637404bac0c0acdb4bfd2da3a3076ed55f75cea9 Mon Sep 17 00:00:00 2001 From: Andrey Marchenko Date: Fri, 5 Jan 2024 16:12:16 +0100 Subject: [PATCH 02/13] add more examples from github CODEOWNERS format docs and fix my pattern matcher --- lib/datadog/ci/codeowners/matcher.rb | 23 +++- lib/datadog/ci/codeowners/rule.rb | 17 ++- sig/datadog/ci/codeowners/matcher.rbs | 2 + sig/datadog/ci/codeowners/rule.rbs | 4 + spec/datadog/ci/codeowners/matcher_spec.rb | 152 ++++++++++++++++++++- 5 files changed, 194 insertions(+), 4 deletions(-) diff --git a/lib/datadog/ci/codeowners/matcher.rb b/lib/datadog/ci/codeowners/matcher.rb index 844fe89a..5ac63534 100644 --- a/lib/datadog/ci/codeowners/matcher.rb +++ b/lib/datadog/ci/codeowners/matcher.rb @@ -5,7 +5,7 @@ module Datadog module CI module Codeowners - # Responsible for matching a file path to a list of owners + # Responsible for matching a test source file path to a list of owners class Matcher def initialize(codeowners_file_path) @rules = parse(codeowners_file_path) @@ -13,6 +13,9 @@ def initialize(codeowners_file_path) end def list_owners(file_path) + # treat all file paths that we check as absolute from the repository root + file_path = "/#{file_path}" unless file_path.start_with?("/") + @rules.each do |rule| return rule.owners if rule.match?(file_path) end @@ -37,7 +40,9 @@ def parse(file_path) next if section?(line) pattern, *line_owners = line.strip.split(/\s+/) - next if pattern.nil? || pattern.empty? || line_owners.empty? + next if pattern.nil? || pattern.empty? + + pattern = expand_pattern(pattern) result << Rule.new(pattern, line_owners) end @@ -53,6 +58,20 @@ def comment?(line) def section?(line) line.start_with?("[", "^[") && line.end_with?("]") end + + def expand_pattern(pattern) + return pattern if pattern == "*" + + # if pattern ends with a slash then it matches everything deeply nested in this directory + pattern += "**" if pattern.end_with?(::File::SEPARATOR) + + # if pattern doesn't start with a slash then it matches anywhere in the repository + if !pattern.start_with?(::File::SEPARATOR, "**#{::File::SEPARATOR}", "*#{::File::SEPARATOR}") + pattern = "**#{::File::SEPARATOR}#{pattern}" + end + + pattern + end end end end diff --git a/lib/datadog/ci/codeowners/rule.rb b/lib/datadog/ci/codeowners/rule.rb index c2f562eb..d583db8c 100644 --- a/lib/datadog/ci/codeowners/rule.rb +++ b/lib/datadog/ci/codeowners/rule.rb @@ -10,7 +10,22 @@ def initialize(pattern, owners) end def match?(file_path) - File.fnmatch?(pattern, file_path) + res = false + if !pattern.end_with?(::File::SEPARATOR, "*") && !pattern.include?(".") + # could be a directory + directory_pattern = pattern + "#{::File::SEPARATOR}*" + res ||= File.fnmatch?(directory_pattern, file_path, flags) + end + + res ||= File.fnmatch?(pattern, file_path, flags) + res + end + + private + + def flags + return ::File::FNM_PATHNAME if pattern.end_with?("#{::File::SEPARATOR}*") + 0 end end end diff --git a/sig/datadog/ci/codeowners/matcher.rbs b/sig/datadog/ci/codeowners/matcher.rbs index 1bc1925f..8a33415b 100644 --- a/sig/datadog/ci/codeowners/matcher.rbs +++ b/sig/datadog/ci/codeowners/matcher.rbs @@ -15,6 +15,8 @@ module Datadog def comment?: (String line) -> bool def section?: (String line) -> bool + + def expand_pattern: (String pattern) -> String end end end diff --git a/sig/datadog/ci/codeowners/rule.rbs b/sig/datadog/ci/codeowners/rule.rbs index 618e2dc6..92026461 100644 --- a/sig/datadog/ci/codeowners/rule.rbs +++ b/sig/datadog/ci/codeowners/rule.rbs @@ -13,6 +13,10 @@ module Datadog def initialize: (String pattern, Array[String] owners) -> void def match?: (String file_path) -> untyped + + private + + def flags: () -> Integer end end end diff --git a/spec/datadog/ci/codeowners/matcher_spec.rb b/spec/datadog/ci/codeowners/matcher_spec.rb index e2de09c9..2405b887 100644 --- a/spec/datadog/ci/codeowners/matcher_spec.rb +++ b/spec/datadog/ci/codeowners/matcher_spec.rb @@ -1,6 +1,9 @@ require "datadog/ci/codeowners/matcher" RSpec.describe Datadog::CI::Codeowners::Matcher do + # most of the examples here are from the following sources: + # https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners + # https://docs.gitlab.com/ee/user/project/codeowners/reference.html let(:codeowners_file_path) { "/path/to/codeowners" } let(:matcher) { described_class.new(codeowners_file_path) } @@ -68,7 +71,7 @@ [section1] /path/to/*.rb @owner2 - [section2] + [section2][2] /path/to/file.rb @owner1 CODEOWNERS end @@ -78,5 +81,152 @@ expect(matcher.list_owners("/path/to/another_file.rb")).to eq(["@owner2"]) end end + + context "with global pattern" do + let(:codeowners_content) do + <<-CODEOWNERS + * @owner1 + /path/to/file.rb @owner2 + CODEOWNERS + end + + it "returns the list of owners for the matching pattern" do + expect(matcher.list_owners("/path/to/file.rb")).to eq(["@owner2"]) + expect(matcher.list_owners("/path/to/another_file.rb")).to eq(["@owner1"]) + end + end + + context "with file extension patterns" do + let(:codeowners_content) do + <<-CODEOWNERS + *.js @jsowner + *.go @Datadog/goowner + CODEOWNERS + end + + it "returns the list of owners for the matching pattern" do + expect(matcher.list_owners("/path/to/file.js")).to eq(["@jsowner"]) + expect(matcher.list_owners("main.go")).to eq(["@Datadog/goowner"]) + end + end + + context "when matching directory and all subdirectories" do + let(:codeowners_content) do + <<-CODEOWNERS + * @owner + + # In this example, @buildlogsowner owns any files in the build/logs + # directory at the root of the repository and any of its + # subdirectories. + /build/logs/ @buildlogsowner + CODEOWNERS + end + + it "returns the list of owners for the matching pattern" do + expect(matcher.list_owners("/build/logs/logs.txt")).to eq(["@buildlogsowner"]) + expect(matcher.list_owners("build/logs/2022/logs.txt")).to eq(["@buildlogsowner"]) + expect(matcher.list_owners("/build/logs/2022/12/logs.txt")).to eq(["@buildlogsowner"]) + expect(matcher.list_owners("build/logs/2022/12/logs.txt")).to eq(["@buildlogsowner"]) + + expect(matcher.list_owners("/service/build/logs/logs.txt")).to eq(["@owner"]) + expect(matcher.list_owners("service/build/build.pkg")).to eq(["@owner"]) + end + end + + context "when matching files in a directory but not in subdirectories" do + let(:codeowners_content) do + <<-CODEOWNERS + * @owner + + # The `docs/*` pattern will match files like + # `docs/getting-started.md` but not further nested files like + # `docs/build-app/troubleshooting.md`. + docs/* docs@example.com + CODEOWNERS + end + + it "returns the list of owners for the matching pattern" do + expect(matcher.list_owners("docs/getting-started.md")).to eq(["docs@example.com"]) + expect(matcher.list_owners("docs/build-app/troubleshooting.md")).to eq(["@owner"]) + + expect(matcher.list_owners("some/folder/docs/getting-started.md")).to eq(["docs@example.com"]) + expect(matcher.list_owners("some/folder/docs/build-app/troubleshooting.md")).to eq(["@owner"]) + + expect(matcher.list_owners("/root/docs/getting-started.md")).to eq(["docs@example.com"]) + expect(matcher.list_owners("/root/folder/docs/build-app/troubleshooting.md")).to eq(["@owner"]) + end + end + + context "when matching files in any subdirectory anywhere in the repository" do + let(:codeowners_content) do + <<-CODEOWNERS + * @owner + + # In this example, @octocat owns any file in an apps directory + # anywhere in your repository. + apps/ @octocat + CODEOWNERS + end + + it "returns the list of owners for the matching pattern" do + expect(matcher.list_owners("/apps/file.txt")).to eq(["@octocat"]) + expect(matcher.list_owners("/some/folder/apps/file.txt")).to eq(["@octocat"]) + expect(matcher.list_owners("some/folder/apps/1/file.txt")).to eq(["@octocat"]) + expect(matcher.list_owners("some/folder/apps/1/2/file.txt")).to eq(["@octocat"]) + + expect(matcher.list_owners("file.txt")).to eq(["@owner"]) + expect(matcher.list_owners("/file.txt")).to eq(["@owner"]) + expect(matcher.list_owners("some/folder/file.txt")).to eq(["@owner"]) + end + end + + context "when pattern starts from **" do + let(:codeowners_content) do + <<-CODEOWNERS + * @owner + + # In this example, @octocat owns any file in a `/logs` directory such as + # `/build/logs`, `/scripts/logs`, and `/deeply/nested/logs`. Any changes + # in a `/logs` directory will require approval from @octocat. + **/logs @octocat + CODEOWNERS + end + + it "returns the list of owners for the matching pattern" do + expect(matcher.list_owners("/build/logs/logs.txt")).to eq(["@octocat"]) + expect(matcher.list_owners("/scripts/logs/logs.txt")).to eq(["@octocat"]) + expect(matcher.list_owners("/deeply/nested/logs/logs.txt")).to eq(["@octocat"]) + expect(matcher.list_owners("/logs/logs.txt")).to eq(["@octocat"]) + + expect(matcher.list_owners("file.txt")).to eq(["@owner"]) + expect(matcher.list_owners("/file.txt")).to eq(["@owner"]) + expect(matcher.list_owners("some/folder/file.txt")).to eq(["@owner"]) + end + end + + context "when matching anywhere in directory but not in specific subdirectory" do + let(:codeowners_content) do + <<-CODEOWNERS + * @owner + + # In this example, @octocat owns any file in the `/apps` + # directory in the root of your repository except for the `/apps/github` + # subdirectory, as its owners are left empty. + /apps/ @octocat + /apps/github + CODEOWNERS + end + + it "returns the list of owners for the matching pattern" do + expect(matcher.list_owners("/apps/logs.txt")).to eq(["@octocat"]) + expect(matcher.list_owners("/apps/1/logs.txt")).to eq(["@octocat"]) + expect(matcher.list_owners("/apps/deeply/nested/logs/logs.txt")).to eq(["@octocat"]) + + expect(matcher.list_owners("apps/github")).to eq([]) + expect(matcher.list_owners("apps/github/codeowners")).to eq([]) + + expect(matcher.list_owners("other/file.txt")).to eq(["@owner"]) + end + end end end From 0fce6f12f5b311aa44c1de1626e91c3103f3ef0f Mon Sep 17 00:00:00 2001 From: Andrey Marchenko Date: Fri, 5 Jan 2024 16:28:32 +0100 Subject: [PATCH 03/13] return nil if codeowners not found to distinguish cases when there are no codeowners and when the file configured to have no owners --- lib/datadog/ci/codeowners/matcher.rb | 2 +- lib/datadog/ci/codeowners/rule.rb | 4 ++-- sig/datadog/ci/codeowners/matcher.rbs | 2 +- spec/datadog/ci/codeowners/matcher_spec.rb | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/datadog/ci/codeowners/matcher.rb b/lib/datadog/ci/codeowners/matcher.rb index 5ac63534..61fb0630 100644 --- a/lib/datadog/ci/codeowners/matcher.rb +++ b/lib/datadog/ci/codeowners/matcher.rb @@ -20,7 +20,7 @@ def list_owners(file_path) return rule.owners if rule.match?(file_path) end - [] + nil end private diff --git a/lib/datadog/ci/codeowners/rule.rb b/lib/datadog/ci/codeowners/rule.rb index d583db8c..6652b29f 100644 --- a/lib/datadog/ci/codeowners/rule.rb +++ b/lib/datadog/ci/codeowners/rule.rb @@ -11,8 +11,8 @@ def initialize(pattern, owners) def match?(file_path) res = false - if !pattern.end_with?(::File::SEPARATOR, "*") && !pattern.include?(".") - # could be a directory + # if pattern does not end with a separator or a wildcard, it could be either a directory or a file + if !pattern.end_with?(::File::SEPARATOR, "*") directory_pattern = pattern + "#{::File::SEPARATOR}*" res ||= File.fnmatch?(directory_pattern, file_path, flags) end diff --git a/sig/datadog/ci/codeowners/matcher.rbs b/sig/datadog/ci/codeowners/matcher.rbs index 8a33415b..28aacf55 100644 --- a/sig/datadog/ci/codeowners/matcher.rbs +++ b/sig/datadog/ci/codeowners/matcher.rbs @@ -6,7 +6,7 @@ module Datadog def initialize: (String codeowners_file_path) -> void - def list_owners: (String file_path) -> Array[String] + def list_owners: (String file_path) -> Array[String]? private diff --git a/spec/datadog/ci/codeowners/matcher_spec.rb b/spec/datadog/ci/codeowners/matcher_spec.rb index 2405b887..49d63404 100644 --- a/spec/datadog/ci/codeowners/matcher_spec.rb +++ b/spec/datadog/ci/codeowners/matcher_spec.rb @@ -17,7 +17,7 @@ let(:codeowners_content) { "" } it "returns an empty array" do - expect(matcher.list_owners("file.rb")).to eq([]) + expect(matcher.list_owners("file.rb")).to be_nil end end @@ -44,7 +44,7 @@ end it "returns an empty array" do - expect(matcher.list_owners("/path/to/another_file.rb")).to eq([]) + expect(matcher.list_owners("/path/to/another_file.rb")).to be_nil end end From 625a028af51fb703755081819caed7c7a9c56978 Mon Sep 17 00:00:00 2001 From: Andrey Marchenko Date: Fri, 5 Jan 2024 16:39:04 +0100 Subject: [PATCH 04/13] rescue from exceptions when opening CODEOWNERS file if they occur --- lib/datadog/ci/codeowners/matcher.rb | 12 +- spec/datadog/ci/codeowners/matcher_spec.rb | 432 +++++++++++---------- 2 files changed, 231 insertions(+), 213 deletions(-) diff --git a/lib/datadog/ci/codeowners/matcher.rb b/lib/datadog/ci/codeowners/matcher.rb index 61fb0630..cd96d1a4 100644 --- a/lib/datadog/ci/codeowners/matcher.rb +++ b/lib/datadog/ci/codeowners/matcher.rb @@ -25,12 +25,12 @@ def list_owners(file_path) private - def parse(file_path) - return [] unless File.exist?(file_path) + def parse(codeowners_file_path) + return [] unless File.exist?(codeowners_file_path) result = [] - File.open(file_path, "r") do |f| + File.open(codeowners_file_path, "r") do |f| f.each_line do |line| line.strip! @@ -49,6 +49,12 @@ def parse(file_path) end result + rescue => e + Datadog.logger.warn( + "Failed to parse codeowners file at #{codeowners_file_path}: " \ + "#{e.class.name} #{e.message} at #{Array(e.backtrace).first}" + ) + [] end def comment?(line) diff --git a/spec/datadog/ci/codeowners/matcher_spec.rb b/spec/datadog/ci/codeowners/matcher_spec.rb index 49d63404..3b93acce 100644 --- a/spec/datadog/ci/codeowners/matcher_spec.rb +++ b/spec/datadog/ci/codeowners/matcher_spec.rb @@ -7,225 +7,237 @@ let(:codeowners_file_path) { "/path/to/codeowners" } let(:matcher) { described_class.new(codeowners_file_path) } - before do - allow(File).to receive(:exist?).with(codeowners_file_path).and_return(true) - allow(File).to receive(:open).with(codeowners_file_path, "r").and_yield(StringIO.new(codeowners_content)) - end - describe "#list_owners" do - context "when the codeowners file is empty" do - let(:codeowners_content) { "" } - - it "returns an empty array" do - expect(matcher.list_owners("file.rb")).to be_nil + context "provided codeowners path does not exist" do + before do + allow(File).to receive(:exist?).with(codeowners_file_path).and_return(false) end - end - context "when the codeowners file contains matching patterns" do - let(:codeowners_content) do - <<-CODEOWNERS - # Comment line - /path/to/*.rb @owner3 - /path/to/file.rb @owner1 @owner2 - CODEOWNERS - end - - it "returns the list of owners for the matching pattern" do - expect(matcher.list_owners("/path/to/file.rb")).to eq(["@owner1", "@owner2"]) - expect(matcher.list_owners("/path/to/another_file.rb")).to eq(["@owner3"]) - end - end - - context "when the codeowners file contains non-matching patterns" do - let(:codeowners_content) do - <<-CODEOWNERS - /path/to/file.rb @owner1 - CODEOWNERS - end - - it "returns an empty array" do - expect(matcher.list_owners("/path/to/another_file.rb")).to be_nil - end - end - - context "when the codeowners file contains comments and empty lines" do - let(:codeowners_content) do - <<-CODEOWNERS - # Comment line - /path/to/*.rb @owner2 - - # Another comment line - /path/to/file.rb @owner1 - CODEOWNERS - end - - it "returns the list of owners for the matching pattern" do - expect(matcher.list_owners("/path/to/file.rb")).to eq(["@owner1"]) - expect(matcher.list_owners("/path/to/another_file.rb")).to eq(["@owner2"]) - end - end - - context "when the codeowners file contains section lines" do - let(:codeowners_content) do - <<-CODEOWNERS - [section1] - /path/to/*.rb @owner2 - - [section2][2] - /path/to/file.rb @owner1 - CODEOWNERS - end - - it "returns the list of owners for the matching pattern" do - expect(matcher.list_owners("/path/to/file.rb")).to eq(["@owner1"]) - expect(matcher.list_owners("/path/to/another_file.rb")).to eq(["@owner2"]) - end - end - - context "with global pattern" do - let(:codeowners_content) do - <<-CODEOWNERS - * @owner1 - /path/to/file.rb @owner2 - CODEOWNERS - end - - it "returns the list of owners for the matching pattern" do - expect(matcher.list_owners("/path/to/file.rb")).to eq(["@owner2"]) - expect(matcher.list_owners("/path/to/another_file.rb")).to eq(["@owner1"]) - end - end - - context "with file extension patterns" do - let(:codeowners_content) do - <<-CODEOWNERS - *.js @jsowner - *.go @Datadog/goowner - CODEOWNERS - end - - it "returns the list of owners for the matching pattern" do - expect(matcher.list_owners("/path/to/file.js")).to eq(["@jsowner"]) - expect(matcher.list_owners("main.go")).to eq(["@Datadog/goowner"]) - end - end - - context "when matching directory and all subdirectories" do - let(:codeowners_content) do - <<-CODEOWNERS - * @owner - - # In this example, @buildlogsowner owns any files in the build/logs - # directory at the root of the repository and any of its - # subdirectories. - /build/logs/ @buildlogsowner - CODEOWNERS - end - - it "returns the list of owners for the matching pattern" do - expect(matcher.list_owners("/build/logs/logs.txt")).to eq(["@buildlogsowner"]) - expect(matcher.list_owners("build/logs/2022/logs.txt")).to eq(["@buildlogsowner"]) - expect(matcher.list_owners("/build/logs/2022/12/logs.txt")).to eq(["@buildlogsowner"]) - expect(matcher.list_owners("build/logs/2022/12/logs.txt")).to eq(["@buildlogsowner"]) - - expect(matcher.list_owners("/service/build/logs/logs.txt")).to eq(["@owner"]) - expect(matcher.list_owners("service/build/build.pkg")).to eq(["@owner"]) - end - end - - context "when matching files in a directory but not in subdirectories" do - let(:codeowners_content) do - <<-CODEOWNERS - * @owner - - # The `docs/*` pattern will match files like - # `docs/getting-started.md` but not further nested files like - # `docs/build-app/troubleshooting.md`. - docs/* docs@example.com - CODEOWNERS - end - - it "returns the list of owners for the matching pattern" do - expect(matcher.list_owners("docs/getting-started.md")).to eq(["docs@example.com"]) - expect(matcher.list_owners("docs/build-app/troubleshooting.md")).to eq(["@owner"]) - - expect(matcher.list_owners("some/folder/docs/getting-started.md")).to eq(["docs@example.com"]) - expect(matcher.list_owners("some/folder/docs/build-app/troubleshooting.md")).to eq(["@owner"]) - - expect(matcher.list_owners("/root/docs/getting-started.md")).to eq(["docs@example.com"]) - expect(matcher.list_owners("/root/folder/docs/build-app/troubleshooting.md")).to eq(["@owner"]) - end - end - - context "when matching files in any subdirectory anywhere in the repository" do - let(:codeowners_content) do - <<-CODEOWNERS - * @owner - - # In this example, @octocat owns any file in an apps directory - # anywhere in your repository. - apps/ @octocat - CODEOWNERS - end - - it "returns the list of owners for the matching pattern" do - expect(matcher.list_owners("/apps/file.txt")).to eq(["@octocat"]) - expect(matcher.list_owners("/some/folder/apps/file.txt")).to eq(["@octocat"]) - expect(matcher.list_owners("some/folder/apps/1/file.txt")).to eq(["@octocat"]) - expect(matcher.list_owners("some/folder/apps/1/2/file.txt")).to eq(["@octocat"]) - - expect(matcher.list_owners("file.txt")).to eq(["@owner"]) - expect(matcher.list_owners("/file.txt")).to eq(["@owner"]) - expect(matcher.list_owners("some/folder/file.txt")).to eq(["@owner"]) - end - end - - context "when pattern starts from **" do - let(:codeowners_content) do - <<-CODEOWNERS - * @owner - - # In this example, @octocat owns any file in a `/logs` directory such as - # `/build/logs`, `/scripts/logs`, and `/deeply/nested/logs`. Any changes - # in a `/logs` directory will require approval from @octocat. - **/logs @octocat - CODEOWNERS - end - - it "returns the list of owners for the matching pattern" do - expect(matcher.list_owners("/build/logs/logs.txt")).to eq(["@octocat"]) - expect(matcher.list_owners("/scripts/logs/logs.txt")).to eq(["@octocat"]) - expect(matcher.list_owners("/deeply/nested/logs/logs.txt")).to eq(["@octocat"]) - expect(matcher.list_owners("/logs/logs.txt")).to eq(["@octocat"]) - - expect(matcher.list_owners("file.txt")).to eq(["@owner"]) - expect(matcher.list_owners("/file.txt")).to eq(["@owner"]) - expect(matcher.list_owners("some/folder/file.txt")).to eq(["@owner"]) + it "always returns nil" do + expect(matcher.list_owners("file.rb")).to be_nil end end - context "when matching anywhere in directory but not in specific subdirectory" do - let(:codeowners_content) do - <<-CODEOWNERS - * @owner - - # In this example, @octocat owns any file in the `/apps` - # directory in the root of your repository except for the `/apps/github` - # subdirectory, as its owners are left empty. - /apps/ @octocat - /apps/github - CODEOWNERS + context "provided codeowners path exists" do + before do + allow(File).to receive(:exist?).with(codeowners_file_path).and_return(true) + allow(File).to receive(:open).with(codeowners_file_path, "r").and_yield(StringIO.new(codeowners_content)) end - it "returns the list of owners for the matching pattern" do - expect(matcher.list_owners("/apps/logs.txt")).to eq(["@octocat"]) - expect(matcher.list_owners("/apps/1/logs.txt")).to eq(["@octocat"]) - expect(matcher.list_owners("/apps/deeply/nested/logs/logs.txt")).to eq(["@octocat"]) - - expect(matcher.list_owners("apps/github")).to eq([]) - expect(matcher.list_owners("apps/github/codeowners")).to eq([]) + context "when the codeowners file is empty" do + let(:codeowners_content) { "" } - expect(matcher.list_owners("other/file.txt")).to eq(["@owner"]) + it "returns an empty array" do + expect(matcher.list_owners("file.rb")).to be_nil + end + end + + context "when the codeowners file contains matching patterns" do + let(:codeowners_content) do + <<-CODEOWNERS + # Comment line + /path/to/*.rb @owner3 + /path/to/file.rb @owner1 @owner2 + CODEOWNERS + end + + it "returns the list of owners for the matching pattern" do + expect(matcher.list_owners("/path/to/file.rb")).to eq(["@owner1", "@owner2"]) + expect(matcher.list_owners("/path/to/another_file.rb")).to eq(["@owner3"]) + end + end + + context "when the codeowners file contains non-matching patterns" do + let(:codeowners_content) do + <<-CODEOWNERS + /path/to/file.rb @owner1 + CODEOWNERS + end + + it "returns an empty array" do + expect(matcher.list_owners("/path/to/another_file.rb")).to be_nil + end + end + + context "when the codeowners file contains comments and empty lines" do + let(:codeowners_content) do + <<-CODEOWNERS + # Comment line + /path/to/*.rb @owner2 + + # Another comment line + /path/to/file.rb @owner1 + CODEOWNERS + end + + it "returns the list of owners for the matching pattern" do + expect(matcher.list_owners("/path/to/file.rb")).to eq(["@owner1"]) + expect(matcher.list_owners("/path/to/another_file.rb")).to eq(["@owner2"]) + end + end + + context "when the codeowners file contains section lines" do + let(:codeowners_content) do + <<-CODEOWNERS + [section1] + /path/to/*.rb @owner2 + + [section2][2] + /path/to/file.rb @owner1 + CODEOWNERS + end + + it "returns the list of owners for the matching pattern" do + expect(matcher.list_owners("/path/to/file.rb")).to eq(["@owner1"]) + expect(matcher.list_owners("/path/to/another_file.rb")).to eq(["@owner2"]) + end + end + + context "with global pattern" do + let(:codeowners_content) do + <<-CODEOWNERS + * @owner1 + /path/to/file.rb @owner2 + CODEOWNERS + end + + it "returns the list of owners for the matching pattern" do + expect(matcher.list_owners("/path/to/file.rb")).to eq(["@owner2"]) + expect(matcher.list_owners("/path/to/another_file.rb")).to eq(["@owner1"]) + end + end + + context "with file extension patterns" do + let(:codeowners_content) do + <<-CODEOWNERS + *.js @jsowner + *.go @Datadog/goowner + CODEOWNERS + end + + it "returns the list of owners for the matching pattern" do + expect(matcher.list_owners("/path/to/file.js")).to eq(["@jsowner"]) + expect(matcher.list_owners("main.go")).to eq(["@Datadog/goowner"]) + end + end + + context "when matching directory and all subdirectories" do + let(:codeowners_content) do + <<-CODEOWNERS + * @owner + + # In this example, @buildlogsowner owns any files in the build/logs + # directory at the root of the repository and any of its + # subdirectories. + /build/logs/ @buildlogsowner + CODEOWNERS + end + + it "returns the list of owners for the matching pattern" do + expect(matcher.list_owners("/build/logs/logs.txt")).to eq(["@buildlogsowner"]) + expect(matcher.list_owners("build/logs/2022/logs.txt")).to eq(["@buildlogsowner"]) + expect(matcher.list_owners("/build/logs/2022/12/logs.txt")).to eq(["@buildlogsowner"]) + expect(matcher.list_owners("build/logs/2022/12/logs.txt")).to eq(["@buildlogsowner"]) + + expect(matcher.list_owners("/service/build/logs/logs.txt")).to eq(["@owner"]) + expect(matcher.list_owners("service/build/build.pkg")).to eq(["@owner"]) + end + end + + context "when matching files in a directory but not in subdirectories" do + let(:codeowners_content) do + <<-CODEOWNERS + * @owner + + # The `docs/*` pattern will match files like + # `docs/getting-started.md` but not further nested files like + # `docs/build-app/troubleshooting.md`. + docs/* docs@example.com + CODEOWNERS + end + + it "returns the list of owners for the matching pattern" do + expect(matcher.list_owners("docs/getting-started.md")).to eq(["docs@example.com"]) + expect(matcher.list_owners("docs/build-app/troubleshooting.md")).to eq(["@owner"]) + + expect(matcher.list_owners("some/folder/docs/getting-started.md")).to eq(["docs@example.com"]) + expect(matcher.list_owners("some/folder/docs/build-app/troubleshooting.md")).to eq(["@owner"]) + + expect(matcher.list_owners("/root/docs/getting-started.md")).to eq(["docs@example.com"]) + expect(matcher.list_owners("/root/folder/docs/build-app/troubleshooting.md")).to eq(["@owner"]) + end + end + + context "when matching files in any subdirectory anywhere in the repository" do + let(:codeowners_content) do + <<-CODEOWNERS + * @owner + + # In this example, @octocat owns any file in an apps directory + # anywhere in your repository. + apps/ @octocat + CODEOWNERS + end + + it "returns the list of owners for the matching pattern" do + expect(matcher.list_owners("/apps/file.txt")).to eq(["@octocat"]) + expect(matcher.list_owners("/some/folder/apps/file.txt")).to eq(["@octocat"]) + expect(matcher.list_owners("some/folder/apps/1/file.txt")).to eq(["@octocat"]) + expect(matcher.list_owners("some/folder/apps/1/2/file.txt")).to eq(["@octocat"]) + + expect(matcher.list_owners("file.txt")).to eq(["@owner"]) + expect(matcher.list_owners("/file.txt")).to eq(["@owner"]) + expect(matcher.list_owners("some/folder/file.txt")).to eq(["@owner"]) + end + end + + context "when pattern starts from **" do + let(:codeowners_content) do + <<-CODEOWNERS + * @owner + + # In this example, @octocat owns any file in a `/logs` directory such as + # `/build/logs`, `/scripts/logs`, and `/deeply/nested/logs`. Any changes + # in a `/logs` directory will require approval from @octocat. + **/logs @octocat + CODEOWNERS + end + + it "returns the list of owners for the matching pattern" do + expect(matcher.list_owners("/build/logs/logs.txt")).to eq(["@octocat"]) + expect(matcher.list_owners("/scripts/logs/logs.txt")).to eq(["@octocat"]) + expect(matcher.list_owners("/deeply/nested/logs/logs.txt")).to eq(["@octocat"]) + expect(matcher.list_owners("/logs/logs.txt")).to eq(["@octocat"]) + + expect(matcher.list_owners("file.txt")).to eq(["@owner"]) + expect(matcher.list_owners("/file.txt")).to eq(["@owner"]) + expect(matcher.list_owners("some/folder/file.txt")).to eq(["@owner"]) + end + end + + context "when matching anywhere in directory but not in specific subdirectory" do + let(:codeowners_content) do + <<-CODEOWNERS + * @owner + + # In this example, @octocat owns any file in the `/apps` + # directory in the root of your repository except for the `/apps/github` + # subdirectory, as its owners are left empty. + /apps/ @octocat + /apps/github + CODEOWNERS + end + + it "returns the list of owners for the matching pattern" do + expect(matcher.list_owners("/apps/logs.txt")).to eq(["@octocat"]) + expect(matcher.list_owners("/apps/1/logs.txt")).to eq(["@octocat"]) + expect(matcher.list_owners("/apps/deeply/nested/logs/logs.txt")).to eq(["@octocat"]) + + expect(matcher.list_owners("apps/github")).to eq([]) + expect(matcher.list_owners("apps/github/codeowners")).to eq([]) + + expect(matcher.list_owners("other/file.txt")).to eq(["@owner"]) + end end end end From 407434f58b006e2abcd20a7becc0a1d606e95602 Mon Sep 17 00:00:00 2001 From: Andrey Marchenko Date: Mon, 8 Jan 2024 10:14:46 +0100 Subject: [PATCH 05/13] support default codeowners for section in gitlab format --- lib/datadog/ci/codeowners/matcher.rb | 13 +++++- spec/datadog/ci/codeowners/matcher_spec.rb | 51 ++++++++++++++++++---- 2 files changed, 53 insertions(+), 11 deletions(-) diff --git a/lib/datadog/ci/codeowners/matcher.rb b/lib/datadog/ci/codeowners/matcher.rb index cd96d1a4..7e8c7741 100644 --- a/lib/datadog/ci/codeowners/matcher.rb +++ b/lib/datadog/ci/codeowners/matcher.rb @@ -29,20 +29,29 @@ def parse(codeowners_file_path) return [] unless File.exist?(codeowners_file_path) result = [] + section_default_owners = [] File.open(codeowners_file_path, "r") do |f| f.each_line do |line| line.strip! - # Skip comments, empty lines, and section lines next if line.empty? next if comment?(line) - next if section?(line) pattern, *line_owners = line.strip.split(/\s+/) next if pattern.nil? || pattern.empty? + # if the current line starts with section record the default owners for this section + if section?(pattern) + section_default_owners = line_owners + next + end + pattern = expand_pattern(pattern) + # if the current line doesn't have any owners then use the default owners for this section + if line_owners.empty? && !section_default_owners.empty? + line_owners = section_default_owners + end result << Rule.new(pattern, line_owners) end diff --git a/spec/datadog/ci/codeowners/matcher_spec.rb b/spec/datadog/ci/codeowners/matcher_spec.rb index 3b93acce..d21dcd9e 100644 --- a/spec/datadog/ci/codeowners/matcher_spec.rb +++ b/spec/datadog/ci/codeowners/matcher_spec.rb @@ -41,7 +41,7 @@ CODEOWNERS end - it "returns the list of owners for the matching pattern" do + it "returns the list of owners" do expect(matcher.list_owners("/path/to/file.rb")).to eq(["@owner1", "@owner2"]) expect(matcher.list_owners("/path/to/another_file.rb")).to eq(["@owner3"]) end @@ -70,7 +70,7 @@ CODEOWNERS end - it "returns the list of owners for the matching pattern" do + it "returns the list of owners" do expect(matcher.list_owners("/path/to/file.rb")).to eq(["@owner1"]) expect(matcher.list_owners("/path/to/another_file.rb")).to eq(["@owner2"]) end @@ -87,7 +87,7 @@ CODEOWNERS end - it "returns the list of owners for the matching pattern" do + it "returns the list of owners" do expect(matcher.list_owners("/path/to/file.rb")).to eq(["@owner1"]) expect(matcher.list_owners("/path/to/another_file.rb")).to eq(["@owner2"]) end @@ -112,12 +112,16 @@ <<-CODEOWNERS *.js @jsowner *.go @Datadog/goowner + *.java CODEOWNERS end - it "returns the list of owners for the matching pattern" do + it "returns the list of owners" do expect(matcher.list_owners("/path/to/file.js")).to eq(["@jsowner"]) expect(matcher.list_owners("main.go")).to eq(["@Datadog/goowner"]) + expect(matcher.list_owners("/main.go")).to eq(["@Datadog/goowner"]) + expect(matcher.list_owners("file.rb")).to be_nil + expect(matcher.list_owners("AbstractFactory.java")).to eq([]) end end @@ -133,7 +137,7 @@ CODEOWNERS end - it "returns the list of owners for the matching pattern" do + it "returns the list of owners" do expect(matcher.list_owners("/build/logs/logs.txt")).to eq(["@buildlogsowner"]) expect(matcher.list_owners("build/logs/2022/logs.txt")).to eq(["@buildlogsowner"]) expect(matcher.list_owners("/build/logs/2022/12/logs.txt")).to eq(["@buildlogsowner"]) @@ -156,7 +160,7 @@ CODEOWNERS end - it "returns the list of owners for the matching pattern" do + it "returns the list of owners" do expect(matcher.list_owners("docs/getting-started.md")).to eq(["docs@example.com"]) expect(matcher.list_owners("docs/build-app/troubleshooting.md")).to eq(["@owner"]) @@ -179,7 +183,7 @@ CODEOWNERS end - it "returns the list of owners for the matching pattern" do + it "returns the list of owners" do expect(matcher.list_owners("/apps/file.txt")).to eq(["@octocat"]) expect(matcher.list_owners("/some/folder/apps/file.txt")).to eq(["@octocat"]) expect(matcher.list_owners("some/folder/apps/1/file.txt")).to eq(["@octocat"]) @@ -203,7 +207,7 @@ CODEOWNERS end - it "returns the list of owners for the matching pattern" do + it "returns the list of owners" do expect(matcher.list_owners("/build/logs/logs.txt")).to eq(["@octocat"]) expect(matcher.list_owners("/scripts/logs/logs.txt")).to eq(["@octocat"]) expect(matcher.list_owners("/deeply/nested/logs/logs.txt")).to eq(["@octocat"]) @@ -228,7 +232,7 @@ CODEOWNERS end - it "returns the list of owners for the matching pattern" do + it "returns the list of owners" do expect(matcher.list_owners("/apps/logs.txt")).to eq(["@octocat"]) expect(matcher.list_owners("/apps/1/logs.txt")).to eq(["@octocat"]) expect(matcher.list_owners("/apps/deeply/nested/logs/logs.txt")).to eq(["@octocat"]) @@ -239,6 +243,35 @@ expect(matcher.list_owners("other/file.txt")).to eq(["@owner"]) end end + + context "when using Gitlab format with default owner per section" do + let(:codeowners_content) do + <<-CODEOWNERS + # Use of default owners for a section. In this case, all files (*) are owned by + # the dev team except the README.md and data-models which are owned by other teams. + [Development] @dev-team + * + README.md @docs-team + data-models/ @data-science-team + + [Testing] + *_spec.rb @qa-team + CODEOWNERS + end + + it "returns the list of owners" do + expect(matcher.list_owners("data-models/model")).to eq(["@data-science-team"]) + expect(matcher.list_owners("data-models/search/model")).to eq(["@data-science-team"]) + + expect(matcher.list_owners("README.md")).to eq(["@docs-team"]) + expect(matcher.list_owners("/README.md")).to eq(["@docs-team"]) + + expect(matcher.list_owners("apps/main.go")).to eq(["@dev-team"]) + expect(matcher.list_owners(".gitignore")).to eq(["@dev-team"]) + + expect(matcher.list_owners("spec/helpers_spec.rb")).to eq(["@qa-team"]) + end + end end end end From f78cb2b600cff3e221d39ec2df7466e9650d76ef Mon Sep 17 00:00:00 2001 From: Andrey Marchenko Date: Mon, 8 Jan 2024 12:35:04 +0100 Subject: [PATCH 06/13] create codeowners parser --- lib/datadog/ci/codeowners/parser.rb | 40 +++++++++++++++ sig/datadog/ci/codeowners/parser.rbs | 17 +++++++ spec/datadog/ci/codeowners/parser_spec.rb | 49 +++++++++++++++++++ .../default-codeowners/.github/CODEOWNERS | 1 + .../codeowners/default-codeowners/CODEOWNERS | 1 + .../docs-codeowners/docs/CODEOWNERS | 1 + .../github-codeowners/.github/CODEOWNERS | 1 + .../github-codeowners/.gitlab/CODEOWNERS | 1 + .../gitlab-codeowners/.gitlab/CODEOWNERS | 1 + .../gitlab-codeowners/docs/CODEOWNERS | 1 + 10 files changed, 113 insertions(+) create mode 100644 lib/datadog/ci/codeowners/parser.rb create mode 100644 sig/datadog/ci/codeowners/parser.rbs create mode 100644 spec/datadog/ci/codeowners/parser_spec.rb create mode 100644 spec/support/fixtures/codeowners/default-codeowners/.github/CODEOWNERS create mode 100644 spec/support/fixtures/codeowners/default-codeowners/CODEOWNERS create mode 100644 spec/support/fixtures/codeowners/docs-codeowners/docs/CODEOWNERS create mode 100644 spec/support/fixtures/codeowners/github-codeowners/.github/CODEOWNERS create mode 100644 spec/support/fixtures/codeowners/github-codeowners/.gitlab/CODEOWNERS create mode 100644 spec/support/fixtures/codeowners/gitlab-codeowners/.gitlab/CODEOWNERS create mode 100644 spec/support/fixtures/codeowners/gitlab-codeowners/docs/CODEOWNERS diff --git a/lib/datadog/ci/codeowners/parser.rb b/lib/datadog/ci/codeowners/parser.rb new file mode 100644 index 00000000..42145c81 --- /dev/null +++ b/lib/datadog/ci/codeowners/parser.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require_relative "matcher" + +module Datadog + module CI + module Codeowners + # Responsible for parsing a CODEOWNERS file + class Parser + DEFAULT_LOCATION = "CODEOWNERS" + POSSIBLE_CODEOWNERS_LOCATIONS = [ + "CODEOWNERS", + ".github/CODEOWNERS", + ".gitlab/CODEOWNERS", + "docs/CODEOWNERS" + ].freeze + + def initialize(root_file_path) + @root_file_path = root_file_path || Dir.pwd + end + + def parse + default_path = File.join(@root_file_path, DEFAULT_LOCATION) + # We are using the first codeowners file that we find or + # default location if nothing is found + # + # Matcher handles it internally and creates a class with + # an empty list of rules if the file is not found + codeowners_file_path = POSSIBLE_CODEOWNERS_LOCATIONS.map do |codeowners_location| + File.join(@root_file_path, codeowners_location) + end.find do |path| + File.exist?(path) + end || default_path + + Matcher.new(codeowners_file_path) + end + end + end + end +end diff --git a/sig/datadog/ci/codeowners/parser.rbs b/sig/datadog/ci/codeowners/parser.rbs new file mode 100644 index 00000000..15f064eb --- /dev/null +++ b/sig/datadog/ci/codeowners/parser.rbs @@ -0,0 +1,17 @@ +module Datadog + module CI + module Codeowners + class Parser + @root_file_path: String + + DEFAULT_LOCATION: "CODEOWNERS" + + POSSIBLE_CODEOWNERS_LOCATIONS: ::Array["CODEOWNERS" | ".github/CODEOWNERS" | ".gitlab/CODEOWNERS" | "docs/CODEOWNERS"] + + def initialize: (String? root_file_path) -> void + + def parse: () -> Datadog::CI::Codeowners::Matcher + end + end + end +end diff --git a/spec/datadog/ci/codeowners/parser_spec.rb b/spec/datadog/ci/codeowners/parser_spec.rb new file mode 100644 index 00000000..75b67cd8 --- /dev/null +++ b/spec/datadog/ci/codeowners/parser_spec.rb @@ -0,0 +1,49 @@ +require "datadog/ci/codeowners/parser" + +RSpec.describe Datadog::CI::Codeowners::Parser do + subject { described_class.new(root_file_path) } + let(:matcher) { subject.parse } + let(:fixtures_location) { "spec/support/fixtures/codeowners" } + + describe "#parse" do + context "when no codeowners file exists" do + let(:root_file_path) { "#{fixtures_location}/no_codeowners" } + + it "returns a Matcher instance with empty set of rules" do + expect(matcher.list_owners("foo/bar.rb")).to be_nil + end + end + + context "when a default codeowners file exists in the root directory" do + let(:root_file_path) { "#{fixtures_location}/default-codeowners" } + + it "returns a Matcher instance with default codeowners file path" do + expect(matcher.list_owners("foo/bar.rb")).to eq(["@default"]) + end + end + + context "when a codeowners file exists in a .github subdirectory" do + let(:root_file_path) { "#{fixtures_location}/github-codeowners" } + + it "returns a Matcher instance with github codeowners file path" do + expect(matcher.list_owners("foo/bar.rb")).to eq(["@github"]) + end + end + + context "when a codeowners file exists in a .gitlab subdirectory" do + let(:root_file_path) { "#{fixtures_location}/gitlab-codeowners" } + + it "returns a Matcher instance with gitlab codeowners file path" do + expect(matcher.list_owners("foo/bar.rb")).to eq(["@gitlab"]) + end + end + + context "when a codeowners file exists in a docs subdirectory" do + let(:root_file_path) { "#{fixtures_location}/docs-codeowners" } + + it "returns a Matcher instance with docs codeowners file path" do + expect(matcher.list_owners("foo/bar.rb")).to eq(["@docs"]) + end + end + end +end diff --git a/spec/support/fixtures/codeowners/default-codeowners/.github/CODEOWNERS b/spec/support/fixtures/codeowners/default-codeowners/.github/CODEOWNERS new file mode 100644 index 00000000..d3ab34d0 --- /dev/null +++ b/spec/support/fixtures/codeowners/default-codeowners/.github/CODEOWNERS @@ -0,0 +1 @@ +* @github \ No newline at end of file diff --git a/spec/support/fixtures/codeowners/default-codeowners/CODEOWNERS b/spec/support/fixtures/codeowners/default-codeowners/CODEOWNERS new file mode 100644 index 00000000..07f3b369 --- /dev/null +++ b/spec/support/fixtures/codeowners/default-codeowners/CODEOWNERS @@ -0,0 +1 @@ +* @default \ No newline at end of file diff --git a/spec/support/fixtures/codeowners/docs-codeowners/docs/CODEOWNERS b/spec/support/fixtures/codeowners/docs-codeowners/docs/CODEOWNERS new file mode 100644 index 00000000..05e90f04 --- /dev/null +++ b/spec/support/fixtures/codeowners/docs-codeowners/docs/CODEOWNERS @@ -0,0 +1 @@ +* @docs \ No newline at end of file diff --git a/spec/support/fixtures/codeowners/github-codeowners/.github/CODEOWNERS b/spec/support/fixtures/codeowners/github-codeowners/.github/CODEOWNERS new file mode 100644 index 00000000..d3ab34d0 --- /dev/null +++ b/spec/support/fixtures/codeowners/github-codeowners/.github/CODEOWNERS @@ -0,0 +1 @@ +* @github \ No newline at end of file diff --git a/spec/support/fixtures/codeowners/github-codeowners/.gitlab/CODEOWNERS b/spec/support/fixtures/codeowners/github-codeowners/.gitlab/CODEOWNERS new file mode 100644 index 00000000..5b454bfd --- /dev/null +++ b/spec/support/fixtures/codeowners/github-codeowners/.gitlab/CODEOWNERS @@ -0,0 +1 @@ +* @gitlab \ No newline at end of file diff --git a/spec/support/fixtures/codeowners/gitlab-codeowners/.gitlab/CODEOWNERS b/spec/support/fixtures/codeowners/gitlab-codeowners/.gitlab/CODEOWNERS new file mode 100644 index 00000000..5b454bfd --- /dev/null +++ b/spec/support/fixtures/codeowners/gitlab-codeowners/.gitlab/CODEOWNERS @@ -0,0 +1 @@ +* @gitlab \ No newline at end of file diff --git a/spec/support/fixtures/codeowners/gitlab-codeowners/docs/CODEOWNERS b/spec/support/fixtures/codeowners/gitlab-codeowners/docs/CODEOWNERS new file mode 100644 index 00000000..05e90f04 --- /dev/null +++ b/spec/support/fixtures/codeowners/gitlab-codeowners/docs/CODEOWNERS @@ -0,0 +1 @@ +* @docs \ No newline at end of file From f3ff07f6700bbb642112766e3c0601c90a387312 Mon Sep 17 00:00:00 2001 From: Andrey Marchenko Date: Mon, 8 Jan 2024 12:41:03 +0100 Subject: [PATCH 07/13] add codeowners matcher to the dependencies of test visibility recorder --- lib/datadog/ci/test_visibility/recorder.rb | 8 +++++++- sig/datadog/ci/test_visibility/recorder.rbs | 3 ++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/datadog/ci/test_visibility/recorder.rb b/lib/datadog/ci/test_visibility/recorder.rb index 74b264fa..5db2c967 100644 --- a/lib/datadog/ci/test_visibility/recorder.rb +++ b/lib/datadog/ci/test_visibility/recorder.rb @@ -8,9 +8,11 @@ require_relative "context/global" require_relative "context/local" +require_relative "../codeowners/parser" require_relative "../ext/app_types" require_relative "../ext/test" require_relative "../ext/environment" +require_relative "../utils/git" require_relative "../span" require_relative "../null_span" @@ -27,12 +29,16 @@ module TestVisibility class Recorder attr_reader :environment_tags, :test_suite_level_visibility_enabled - def initialize(test_suite_level_visibility_enabled: false) + def initialize( + test_suite_level_visibility_enabled: false, + codeowners: Codeowners::Parser.new(Utils::Git.root).parse + ) @test_suite_level_visibility_enabled = test_suite_level_visibility_enabled @environment_tags = Ext::Environment.tags(ENV).freeze @local_context = Context::Local.new @global_context = Context::Global.new + @codeowners = codeowners end def start_test_session(service: nil, tags: {}) diff --git a/sig/datadog/ci/test_visibility/recorder.rbs b/sig/datadog/ci/test_visibility/recorder.rbs index ab56cffc..ea4c7cf4 100644 --- a/sig/datadog/ci/test_visibility/recorder.rbs +++ b/sig/datadog/ci/test_visibility/recorder.rbs @@ -7,13 +7,14 @@ module Datadog @environment_tags: Hash[String, String] @local_context: Datadog::CI::TestVisibility::Context::Local @global_context: Datadog::CI::TestVisibility::Context::Global + @codeowners: Datadog::CI::Codeowners::Matcher @null_span: Datadog::CI::NullSpan attr_reader environment_tags: Hash[String, String] attr_reader test_suite_level_visibility_enabled: bool - def initialize: (?test_suite_level_visibility_enabled: bool) -> void + def initialize: (?test_suite_level_visibility_enabled: bool, ?codeowners: Datadog::CI::Codeowners::Matcher) -> void def trace_test: (String span_name, String test_suite_name, ?service: String?, ?tags: Hash[untyped, untyped]) ?{ (Datadog::CI::Span span) -> untyped } -> untyped From 2631e85c9737bb6889943d35e6357a93ae5525cf Mon Sep 17 00:00:00 2001 From: Andrey Marchenko Date: Mon, 8 Jan 2024 12:47:35 +0100 Subject: [PATCH 08/13] add source_file method to Datadog::CI::Test --- lib/datadog/ci/test.rb | 7 +++++++ sig/datadog/ci/test.rbs | 1 + spec/datadog/ci/test_spec.rb | 13 +++++++++++++ 3 files changed, 21 insertions(+) diff --git a/lib/datadog/ci/test.rb b/lib/datadog/ci/test.rb index d537c7fa..de34c4db 100644 --- a/lib/datadog/ci/test.rb +++ b/lib/datadog/ci/test.rb @@ -50,6 +50,13 @@ def test_module_id def test_session_id get_tag(Ext::Test::TAG_TEST_SESSION_ID) end + + # Source file path of the test relative to git repository root. + # @return [String] the source file path of the test + # @return [nil] if the source file path is not found + def source_file + get_tag(Ext::Test::TAG_SOURCE_FILE) + end end end end diff --git a/sig/datadog/ci/test.rbs b/sig/datadog/ci/test.rbs index 0a685590..41b086d2 100644 --- a/sig/datadog/ci/test.rbs +++ b/sig/datadog/ci/test.rbs @@ -7,6 +7,7 @@ module Datadog def test_suite_name: () -> String? def test_module_id: () -> String? def test_session_id: () -> String? + def source_file: () -> String? end end end diff --git a/spec/datadog/ci/test_spec.rb b/spec/datadog/ci/test_spec.rb index 75d5d642..05fc8ed1 100644 --- a/spec/datadog/ci/test_spec.rb +++ b/spec/datadog/ci/test_spec.rb @@ -92,4 +92,17 @@ it { is_expected.to eq("test session id") } end + + describe "#source_file" do + subject(:source_file) { ci_test.source_file } + let(:ci_test) { described_class.new(tracer_span) } + + before do + allow(tracer_span).to( + receive(:get_tag).with(Datadog::CI::Ext::Test::TAG_SOURCE_FILE).and_return("foo/bar.rb") + ) + end + + it { is_expected.to eq("foo/bar.rb") } + end end From 28d0ea8797ad38bcd630258fc1b091316ebcb342 Mon Sep 17 00:00:00 2001 From: Andrey Marchenko Date: Mon, 8 Jan 2024 12:49:27 +0100 Subject: [PATCH 09/13] add TAG_CODEOWNERS constant --- lib/datadog/ci/ext/test.rb | 1 + sig/datadog/ci/ext/test.rbs | 2 ++ 2 files changed, 3 insertions(+) diff --git a/lib/datadog/ci/ext/test.rb b/lib/datadog/ci/ext/test.rb index 89bcfa5b..1aa0a46c 100644 --- a/lib/datadog/ci/ext/test.rb +++ b/lib/datadog/ci/ext/test.rb @@ -21,6 +21,7 @@ module Test TAG_COMMAND = "test.command" TAG_SOURCE_FILE = "test.source.file" TAG_SOURCE_START = "test.source.start" + TAG_CODEOWNERS = "test.codeowners" TEST_TYPE = "test" diff --git a/sig/datadog/ci/ext/test.rbs b/sig/datadog/ci/ext/test.rbs index 627c83bf..95822b88 100644 --- a/sig/datadog/ci/ext/test.rbs +++ b/sig/datadog/ci/ext/test.rbs @@ -30,6 +30,8 @@ module Datadog TAG_SOURCE_START: String + TAG_CODEOWNERS: String + TAG_TEST_SESSION_ID: String TAG_TEST_MODULE_ID: String From 62bcc3159af5f86a699778fd6b53308476caf073 Mon Sep 17 00:00:00 2001 From: Andrey Marchenko Date: Mon, 8 Jan 2024 13:04:26 +0100 Subject: [PATCH 10/13] send codeowners tag for test events and add CODEOWNERS for this repository --- CODEOWNERS | 1 + lib/datadog/ci/test_visibility/recorder.rb | 8 ++++++++ sig/datadog/ci/test_visibility/recorder.rbs | 2 ++ spec/datadog/ci/contrib/cucumber/instrumentation_spec.rb | 3 +++ spec/datadog/ci/contrib/minitest/instrumentation_spec.rb | 3 +++ spec/datadog/ci/contrib/rspec/instrumentation_spec.rb | 3 +++ 6 files changed, 20 insertions(+) create mode 100644 CODEOWNERS diff --git a/CODEOWNERS b/CODEOWNERS new file mode 100644 index 00000000..3664b2d6 --- /dev/null +++ b/CODEOWNERS @@ -0,0 +1 @@ +* @DataDog/ruby-guild @DataDog/ci-app-libraries \ No newline at end of file diff --git a/lib/datadog/ci/test_visibility/recorder.rb b/lib/datadog/ci/test_visibility/recorder.rb index 5db2c967..74ff429f 100644 --- a/lib/datadog/ci/test_visibility/recorder.rb +++ b/lib/datadog/ci/test_visibility/recorder.rb @@ -212,6 +212,8 @@ def build_test(tracer_span, tags) test = Test.new(tracer_span) set_initial_tags(test, tags) validate_test_suite_level_visibility_correctness(test) + set_codeowners(test) + test end @@ -257,6 +259,12 @@ def set_module_context(tags, test_module = nil) end end + def set_codeowners(test) + source = test.source_file + owners = @codeowners.list_owners(source) if source + test.set_tag(Ext::Test::TAG_CODEOWNERS, owners) unless owners.nil? + end + def set_suite_context(tags, span: nil, name: nil) return if span.nil? && name.nil? diff --git a/sig/datadog/ci/test_visibility/recorder.rbs b/sig/datadog/ci/test_visibility/recorder.rbs index ea4c7cf4..ae75ebb5 100644 --- a/sig/datadog/ci/test_visibility/recorder.rbs +++ b/sig/datadog/ci/test_visibility/recorder.rbs @@ -71,6 +71,8 @@ module Datadog def set_module_context: (Hash[untyped, untyped] tags, ?Datadog::CI::TestModule | Datadog::Tracing::SpanOperation? test_module) -> void + def set_codeowners: (Datadog::CI::Test test) -> void + def null_span: () -> Datadog::CI::Span def skip_tracing: (?untyped block) -> untyped diff --git a/spec/datadog/ci/contrib/cucumber/instrumentation_spec.rb b/spec/datadog/ci/contrib/cucumber/instrumentation_spec.rb index c0615fa6..9f0f26aa 100644 --- a/spec/datadog/ci/contrib/cucumber/instrumentation_spec.rb +++ b/spec/datadog/ci/contrib/cucumber/instrumentation_spec.rb @@ -94,6 +94,9 @@ "spec/datadog/ci/contrib/cucumber/features/passing.feature" ) expect(scenario_span.get_tag(Datadog::CI::Ext::Test::TAG_SOURCE_START)).to eq("3") + expect(scenario_span.get_tag(Datadog::CI::Ext::Test::TAG_CODEOWNERS)).to eq( + "[\"@DataDog/ruby-guild\", \"@DataDog/ci-app-libraries\"]" + ) step_span = spans.find { |s| s.resource == "datadog" } expect(step_span.resource).to eq("datadog") diff --git a/spec/datadog/ci/contrib/minitest/instrumentation_spec.rb b/spec/datadog/ci/contrib/minitest/instrumentation_spec.rb index d30a8c44..3002ab07 100644 --- a/spec/datadog/ci/contrib/minitest/instrumentation_spec.rb +++ b/spec/datadog/ci/contrib/minitest/instrumentation_spec.rb @@ -51,6 +51,9 @@ def test_foo "spec/datadog/ci/contrib/minitest/instrumentation_spec.rb" ) expect(span.get_tag(Datadog::CI::Ext::Test::TAG_SOURCE_START)).to eq("29") + expect(span.get_tag(Datadog::CI::Ext::Test::TAG_CODEOWNERS)).to eq( + "[\"@DataDog/ruby-guild\", \"@DataDog/ci-app-libraries\"]" + ) end it "creates spans for several tests" do diff --git a/spec/datadog/ci/contrib/rspec/instrumentation_spec.rb b/spec/datadog/ci/contrib/rspec/instrumentation_spec.rb index 144f377a..5a56a030 100644 --- a/spec/datadog/ci/contrib/rspec/instrumentation_spec.rb +++ b/spec/datadog/ci/contrib/rspec/instrumentation_spec.rb @@ -47,6 +47,9 @@ def with_new_rspec_environment "spec/datadog/ci/contrib/rspec/instrumentation_spec.rb" ) expect(first_test_span.get_tag(Datadog::CI::Ext::Test::TAG_SOURCE_START)).to eq("26") + expect(first_test_span.get_tag(Datadog::CI::Ext::Test::TAG_CODEOWNERS)).to eq( + "[\"@DataDog/ruby-guild\", \"@DataDog/ci-app-libraries\"]" + ) end it "creates spans for several examples" do From cdcaa4075455324de901555080763d8aa946be39 Mon Sep 17 00:00:00 2001 From: Andrey Marchenko Date: Mon, 8 Jan 2024 13:13:07 +0100 Subject: [PATCH 11/13] add logging to Codeowners files --- lib/datadog/ci/codeowners/matcher.rb | 9 ++++++++- lib/datadog/ci/codeowners/parser.rb | 2 ++ spec/datadog/ci/codeowners/matcher_spec.rb | 4 ++++ spec/datadog/ci/release_gem_spec.rb | 1 + 4 files changed, 15 insertions(+), 1 deletion(-) diff --git a/lib/datadog/ci/codeowners/matcher.rb b/lib/datadog/ci/codeowners/matcher.rb index 7e8c7741..69839c4b 100644 --- a/lib/datadog/ci/codeowners/matcher.rb +++ b/lib/datadog/ci/codeowners/matcher.rb @@ -16,17 +16,24 @@ def list_owners(file_path) # treat all file paths that we check as absolute from the repository root file_path = "/#{file_path}" unless file_path.start_with?("/") + Datadog.logger.debug { "Matching file path #{file_path} to CODEOWNERS rules" } + @rules.each do |rule| + Datadog.logger.debug { "Matched rule [#{rule.pattern}] with owners #{rule.owners}" } return rule.owners if rule.match?(file_path) end + Datadog.logger.debug { "CODEOWNERS rule not matched" } nil end private def parse(codeowners_file_path) - return [] unless File.exist?(codeowners_file_path) + unless File.exist?(codeowners_file_path) + Datadog.logger.debug { "CODEOWNERS file not found at #{codeowners_file_path}" } + return [] + end result = [] section_default_owners = [] diff --git a/lib/datadog/ci/codeowners/parser.rb b/lib/datadog/ci/codeowners/parser.rb index 42145c81..60be8786 100644 --- a/lib/datadog/ci/codeowners/parser.rb +++ b/lib/datadog/ci/codeowners/parser.rb @@ -32,6 +32,8 @@ def parse File.exist?(path) end || default_path + ::Datadog.logger.debug { "Using CODEOWNERS file from: #{codeowners_file_path}" } + Matcher.new(codeowners_file_path) end end diff --git a/spec/datadog/ci/codeowners/matcher_spec.rb b/spec/datadog/ci/codeowners/matcher_spec.rb index d21dcd9e..1687ad56 100644 --- a/spec/datadog/ci/codeowners/matcher_spec.rb +++ b/spec/datadog/ci/codeowners/matcher_spec.rb @@ -7,6 +7,10 @@ let(:codeowners_file_path) { "/path/to/codeowners" } let(:matcher) { described_class.new(codeowners_file_path) } + before do + allow(File).to receive(:exist?).and_return(false) + end + describe "#list_owners" do context "provided codeowners path does not exist" do before do diff --git a/spec/datadog/ci/release_gem_spec.rb b/spec/datadog/ci/release_gem_spec.rb index 5c4df27f..20530394 100644 --- a/spec/datadog/ci/release_gem_spec.rb +++ b/spec/datadog/ci/release_gem_spec.rb @@ -20,6 +20,7 @@ |Appraisals |CODE_OF_CONDUCT.md |CONTRIBUTING.md + |CODEOWNERS |Gemfile |Gemfile-.* |Rakefile From 15d853c169e8ccc2ddab175cb3e6160de76dc86d Mon Sep 17 00:00:00 2001 From: Andrey Marchenko Date: Mon, 8 Jan 2024 13:31:53 +0100 Subject: [PATCH 12/13] do not use unnecessary string concatenation --- lib/datadog/ci/codeowners/rule.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/datadog/ci/codeowners/rule.rb b/lib/datadog/ci/codeowners/rule.rb index 6652b29f..8482253d 100644 --- a/lib/datadog/ci/codeowners/rule.rb +++ b/lib/datadog/ci/codeowners/rule.rb @@ -13,7 +13,7 @@ def match?(file_path) res = false # if pattern does not end with a separator or a wildcard, it could be either a directory or a file if !pattern.end_with?(::File::SEPARATOR, "*") - directory_pattern = pattern + "#{::File::SEPARATOR}*" + directory_pattern = "#{pattern}#{::File::SEPARATOR}*" res ||= File.fnmatch?(directory_pattern, file_path, flags) end From 8e5991186dbd5b3e243c2de2845787371f04e29f Mon Sep 17 00:00:00 2001 From: Andrey Marchenko Date: Mon, 8 Jan 2024 14:22:47 +0100 Subject: [PATCH 13/13] fix logging for codeowners rule matching --- lib/datadog/ci/codeowners/matcher.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/datadog/ci/codeowners/matcher.rb b/lib/datadog/ci/codeowners/matcher.rb index 69839c4b..5b77b7bf 100644 --- a/lib/datadog/ci/codeowners/matcher.rb +++ b/lib/datadog/ci/codeowners/matcher.rb @@ -19,8 +19,10 @@ def list_owners(file_path) Datadog.logger.debug { "Matching file path #{file_path} to CODEOWNERS rules" } @rules.each do |rule| - Datadog.logger.debug { "Matched rule [#{rule.pattern}] with owners #{rule.owners}" } - return rule.owners if rule.match?(file_path) + if rule.match?(file_path) + Datadog.logger.debug { "Matched rule [#{rule.pattern}] with owners #{rule.owners}" } + return rule.owners + end end Datadog.logger.debug { "CODEOWNERS rule not matched" }