Commit 7220b3ab authored by Kerri Miller's avatar Kerri Miller

Remove check for user being applicable codeowner

I mistakenly believed this to be desired behavior, and as part of
cleanup work ahead of a codeowner's project I "fixed" this. I had
thought that we wanted codeowner rules to NOT apply when the user was
listed as an owner of one of the applicable rules, essentially assuming
their approval as implied due to them taking the git action the check is
being applied to. However, this is incorrect, as we still want to push
users towards the MR workflow, even when they are in fact an owner who
_could_ approve the change.
parent 8fbef813
---
title: Remove check for user being an applicable code owner
merge_request: 31768
author:
type: fixed
...@@ -39,7 +39,6 @@ module EE ...@@ -39,7 +39,6 @@ module EE
loader = ::Gitlab::CodeOwners::Loader.new(project, branch_name, paths) loader = ::Gitlab::CodeOwners::Loader.new(project, branch_name, paths)
return if loader.entries.blank? return if loader.entries.blank?
return if loader.members.include?(change_access.user_access.user)
assemble_error_msg_for_codeowner_matches(loader) assemble_error_msg_for_codeowner_matches(loader)
end end
......
...@@ -8,6 +8,12 @@ describe Gitlab::Checks::DiffCheck do ...@@ -8,6 +8,12 @@ describe Gitlab::Checks::DiffCheck do
include_context 'push rules checks context' include_context 'push rules checks context'
describe '#validate!' do describe '#validate!' do
shared_examples_for "returns codeowners validation message" do
it "returns an error message" do
expect(validation_result).to include("Pushes to protected branches")
end
end
context 'no push rules active' do context 'no push rules active' do
let_it_be(:push_rule) { create(:push_rule) } let_it_be(:push_rule) { create(:push_rule) }
...@@ -55,11 +61,6 @@ describe Gitlab::Checks::DiffCheck do ...@@ -55,11 +61,6 @@ describe Gitlab::Checks::DiffCheck do
) )
end end
context 'and the user is not listed as a codeowner' do
before do
stub_feature_flags(sectional_codeowners: false)
end
it "returns an error message" do it "returns an error message" do
expect { diff_check.validate! }.to raise_error do |error| expect { diff_check.validate! }.to raise_error do |error|
expect(error).to be_a(Gitlab::GitAccess::ForbiddenError) expect(error).to be_a(Gitlab::GitAccess::ForbiddenError)
...@@ -68,121 +69,13 @@ describe Gitlab::Checks::DiffCheck do ...@@ -68,121 +69,13 @@ describe Gitlab::Checks::DiffCheck do
end end
end end
context 'and the user is listed as a codeowner' do
# `user` is set as the owner of the incoming change by the shared
# context found in 'push rules checks context'
let(:codeowner_content) { "* @#{user.username}" }
it "does not return an error message" do
expect { diff_check.validate! }.not_to raise_error
end
end
end
context "the MR contains a matching file path" do context "the MR contains a matching file path" do
let(:validation_result) do let(:validation_result) do
subject.send(:validate_code_owners).call(["docs/CODEOWNERS", "README"]) subject.send(:validate_code_owners).call(["docs/CODEOWNERS", "README"])
end end
context "and the user is not listed as a code owner" do
before do
stub_feature_flags(sectional_codeowners: false)
end
context "for a non-web-based request" do
it "returns an error message" do
expect(validation_result).to include("Pushes to protected branches")
expect(validation_result).to include("\n")
end
end
context "for a web-based request" do
before do
allow(subject).to receive(:updated_from_web?).and_return(true)
end
it "returns an error message with newline chars removed" do
expect(validation_result).to include("Pushes to protected branches")
expect(validation_result).not_to include("\n")
end
end
end
context "and the user is listed as a code owner" do
# `user` is set as the owner of the incoming change by the shared
# context found in 'push rules checks context'
let(:codeowner_content) { "* @#{user.username}" }
it "returns nil" do
expect(validation_result).to be_nil
end
end
context "when the codeowner entity is a group" do
let(:group) { create(:group) }
let(:project) { create(:project, :repository, namespace: group) }
let(:codeowner_content) do
<<~CODEOWNERS
*.rb @#{code_owner.username}
docs/CODEOWNERS @#{group.full_path}
*.js.coffee @#{group.full_path}
CODEOWNERS
end
before do
stub_feature_flags(sectional_codeowners: false)
end
shared_examples_for "returns nil" do
it "returns nil" do
expect(validation_result).to be_nil
end
end
shared_examples_for "returns codeowners validation message" do
it "returns an error message" do
expect(validation_result).to include("Pushes to protected branches")
end
end
context "and the project is under a subgroup" do
let(:subgroup) { create(:group, parent: group) }
let(:project) { create(:project, :repository, namespace: subgroup) }
context "and the user is part of the codeowning-group" do
before do
group.add_developer(user)
end
it_behaves_like "returns nil"
end
context "and the user is not part of the codeowning-group" do
it_behaves_like "returns codeowners validation message"
end
context "with :codeowners_match_ancestor_groups disabled" do
before do
stub_feature_flags(codeowners_match_ancestor_groups: false)
end
it_behaves_like "returns codeowners validation message"
end
end
context "and the user is part of the codeowning-group" do
before do
group.add_developer(user)
end
it_behaves_like "returns nil"
end
context "and the user is not part of the codeowning-group" do
it_behaves_like "returns codeowners validation message" it_behaves_like "returns codeowners validation message"
end end
end
end
context "the MR doesn't contain a matching file path" do context "the MR doesn't contain a matching file path" do
it "returns nil" do it "returns nil" do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment