From dbde2b33cda8fde8e75457cc1995b4c6791171a4 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Thu, 16 Aug 2018 18:46:50 +0200 Subject: [PATCH 1/2] Allow specifying code owners in a CODEOWNERS file In a file called `CODEOWNERS` in the root of a repository, in the `docs/`-folder or in the `.gitlab/` folder it is possible to define users that are 'owners' for specific code paths. A pattern can be defined on each line in the same way as it would in a `.gitignore` file. After that, one or more users can be specified using their username (using the `@username` format) or email address linked to their account. Comments can be preceded with a `#`. If a filename starts with `#` this can be escaped using `/#`. For example: # All files in the `docs/` directory should be reviewed by a # technical writer: docs/* @jane @joe # Ruby files should be reviewed by a backend maintainer: *.rb alice@development.gitlab.org The code owners will be displayed when viewing a blob, if a user for the username/email cannot be found, nothing will be shown. When multiple patterns match the blob being viewed, the last entry will be used. --- .gitlab/CODEOWNERS | 15 ++ app/models/blob.rb | 2 + app/models/concerns/case_sensitivity.rb | 45 +++- app/models/user.rb | 5 +- app/views/projects/blob/_blob.html.haml | 2 +- doc/user/project/code_owners.md | 81 +++++++ doc/user/project/index.md | 2 + ee/app/helpers/ee/users_helper.rb | 9 + ee/app/models/ee/blob.rb | 11 + ee/app/models/ee/repository.rb | 5 + ee/app/models/license.rb | 1 + ee/app/views/projects/blob/_owners.html.haml | 9 + .../unreleased/bvl-codeowners-file.yml | 5 + ee/lib/gitlab/code_owners.rb | 16 ++ ee/lib/gitlab/code_owners/file.rb | 92 ++++++++ ee/lib/gitlab/code_owners/loader.rb | 37 ++++ .../view_blob_with_code_owners_spec.rb | 55 +++++ ee/spec/fixtures/codeowners_example | 44 ++++ ee/spec/lib/gitlab/code_owners/file_spec.rb | 164 ++++++++++++++ ee/spec/lib/gitlab/code_owners/loader_spec.rb | 84 ++++++++ ee/spec/lib/gitlab/code_owners_spec.rb | 42 ++++ ee/spec/models/blob_spec.rb | 37 ++++ ee/spec/models/repository_spec.rb | 20 ++ lib/gitlab/user_extractor.rb | 53 +++++ locale/gitlab.pot | 3 + spec/lib/gitlab/user_extractor_spec.rb | 58 +++++ spec/models/concerns/case_sensitivity_spec.rb | 202 +++--------------- spec/models/user_spec.rb | 17 ++ spec/support/helpers/test_env.rb | 3 +- 29 files changed, 935 insertions(+), 184 deletions(-) create mode 100644 .gitlab/CODEOWNERS create mode 100644 doc/user/project/code_owners.md create mode 100644 ee/app/helpers/ee/users_helper.rb create mode 100644 ee/app/models/ee/blob.rb create mode 100644 ee/app/views/projects/blob/_owners.html.haml create mode 100644 ee/changelogs/unreleased/bvl-codeowners-file.yml create mode 100644 ee/lib/gitlab/code_owners.rb create mode 100644 ee/lib/gitlab/code_owners/file.rb create mode 100644 ee/lib/gitlab/code_owners/loader.rb create mode 100644 ee/spec/features/projects/view_blob_with_code_owners_spec.rb create mode 100644 ee/spec/fixtures/codeowners_example create mode 100644 ee/spec/lib/gitlab/code_owners/file_spec.rb create mode 100644 ee/spec/lib/gitlab/code_owners/loader_spec.rb create mode 100644 ee/spec/lib/gitlab/code_owners_spec.rb create mode 100644 ee/spec/models/blob_spec.rb create mode 100644 lib/gitlab/user_extractor.rb create mode 100644 spec/lib/gitlab/user_extractor_spec.rb diff --git a/.gitlab/CODEOWNERS b/.gitlab/CODEOWNERS new file mode 100644 index 00000000000000..5b6e5a719fa56e --- /dev/null +++ b/.gitlab/CODEOWNERS @@ -0,0 +1,15 @@ +# Backend Maintainers are the default for all ruby files +*.rb @ayufan @DouweM @dzaporozhets @grzesiek @nick.thomas @rspeicher @rymai @smcgivern +*.rake @ayufan @DouweM @dzaporozhets @grzesiek @nick.thomas @rspeicher @rymai @smcgivern + +# Technical writing team are the default reviewers for everything in `doc/` +/doc/ @axil @marcia + +# Frontend maintainers should see everything in `app/assets/` +app/assets/ @annabeldunstone @ClemMakesApps @fatihacet @filipa @iamphill @mikegreiling @timzallmann + +# Someone from the database team should review changes in `db/` +db/ @abrandl @NikolayS + +# Feature specific owners +/ee/lib/gitlab/code_owners/ @reprazent diff --git a/app/models/blob.rb b/app/models/blob.rb index acc64ffca67271..34f226c7ef697a 100644 --- a/app/models/blob.rb +++ b/app/models/blob.rb @@ -2,6 +2,8 @@ # Blob is a Rails-specific wrapper around Gitlab::Git::Blob objects class Blob < SimpleDelegator + prepend EE::Blob + CACHE_TIME = 60 # Cache raw blobs referred to by a (mutable) ref for 1 minute CACHE_TIME_IMMUTABLE = 3600 # Cache blobs referred to by an immutable reference for 1 hour diff --git a/app/models/concerns/case_sensitivity.rb b/app/models/concerns/case_sensitivity.rb index 6e80365ee5b808..c93b6589ee7840 100644 --- a/app/models/concerns/case_sensitivity.rb +++ b/app/models/concerns/case_sensitivity.rb @@ -9,23 +9,46 @@ module CaseSensitivity # # Unlike other ActiveRecord methods this method only operates on a Hash. def iwhere(params) - criteria = self - cast_lower = Gitlab::Database.postgresql? + criteria = self params.each do |key, value| - column = ActiveRecord::Base.connection.quote_table_name(key) + criteria = case value + when Array + criteria.where(value_in(key, value)) + else + criteria.where(value_equal(key, value)) + end + end + + criteria + end - condition = - if cast_lower - "LOWER(#{column}) = LOWER(:value)" - else - "#{column} = :value" - end + private + + def value_equal(column, value) + lower_value = lower_value(value) + + lower_column(arel_table[column]).eq(lower_value).to_sql + end - criteria = criteria.where(condition, value: value) + def value_in(column, values) + lower_values = values.map do |value| + lower_value(value) end - criteria + lower_column(arel_table[column]).in(lower_values).to_sql + end + + def lower_value(value) + return value if Gitlab::Database.mysql? + + Arel::Nodes::NamedFunction.new('LOWER', [Arel::Nodes.build_quoted(value)]) + end + + def lower_column(column) + return column if Gitlab::Database.mysql? + + column.lower end end end diff --git a/app/models/user.rb b/app/models/user.rb index 6b749606355ef7..950311bd6f1155 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -266,6 +266,7 @@ def inactive_message scope :order_recent_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('current_sign_in_at', 'DESC')) } scope :order_oldest_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('current_sign_in_at', 'ASC')) } scope :confirmed, -> { where.not(confirmed_at: nil) } + scope :by_username, -> (usernames) { iwhere(username: usernames) } # Limits the users to those that have TODOs, optionally in the given state. # @@ -457,11 +458,11 @@ def by_login(login) end def find_by_username(username) - iwhere(username: username).take + by_username(username).take end def find_by_username!(username) - iwhere(username: username).take! + by_username(username).take! end def find_by_personal_access_token(token_string) diff --git a/app/views/projects/blob/_blob.html.haml b/app/views/projects/blob/_blob.html.haml index a4b1b496b69cde..bb4bbeb6b384fd 100644 --- a/app/views/projects/blob/_blob.html.haml +++ b/app/views/projects/blob/_blob.html.haml @@ -4,7 +4,7 @@ .well-segment %ul.blob-commit-info = render 'projects/commits/commit', commit: @last_commit, project: @project, ref: @ref - + = render_if_exists 'projects/blob/owners', blob: blob = render "projects/blob/auxiliary_viewer", blob: blob #blob-content-holder.blob-content-holder diff --git a/doc/user/project/code_owners.md b/doc/user/project/code_owners.md new file mode 100644 index 00000000000000..8e80c1a21f306d --- /dev/null +++ b/doc/user/project/code_owners.md @@ -0,0 +1,81 @@ +# Code owners + +> [Introduced][introduced-mr] in [Gitlab Starter][ee] 11.3 + +You can use a `CODEOWNERS` file to specify users that are responsible +for a certain part of the code in a repository. + +The codeowners file can be added to the root of the repository, inside +the `.gitlab/` or the `docs/` folder. + +The `CODEOWNERS` is scoped to a branch. This means that with the +introduction of new files, the developer adding the new code can +specify themselves as code owner. Before the new code gets merged to +master. + +## The syntax of a code owners file + +Files can be specified using the same kind of patterns you would use +in the `.gitignore` file followed by the `@username` or email of one +or more users that should be owners of the file. + +The order in which the paths are defined is significant: the last +pattern that matches a given path will be used to find the code +owners. + +Starting a line with a `#` indicates a comment. This needs to be +escaped using `\#` to address files for which the name starts with a +`#`. + +Example `CODEOWNERS` file: + +``` +# This is an example code owners file, lines starting with a `#` will +# be ignored. + +# app/ @commented-rule + +# We can specifiy a default match using wildcards: +* @default-codeowner + +# Rules defined later in the file take precedence over the rules +# defined before. +# This will match all files for which the file name ends in `.rb` +*.rb @ruby-owner + +# Files with a `#` can still be accesssed by escaping the pound sign +\#file_with_pound.rb @owner-file-with-pound + +# Multiple codeowners can be specified, separated by whitespace +CODEOWNERS @multiple @owners @tab-separated + +# Both usernames or email addresses can be used to match +# users. Everything else will be ignored. For example this will +# specify `@legal` and a user with email `janedoe@gitlab.com` as the +# owner for the LICENSE file +LICENSE @legal this does not match janedoe@gitlab.com + +# Ending a path in a `/` will specify the code owners for every file +# nested in that directory, on any level +/docs/ @all-docs + +# Ending a path in `/*` will specify code owners for every file in +# that directory, but not nested deeper. This will match +# `docs/index.md` but not `docs/projects/index.md` +/docs/* @root-docs + +# This will make a `lib` directory nested anywhere in the repository +# match +lib/ @lib-owner + +# This will only match a `config` directory in the root of the +# repository +/config/ @config-owner + + +# If the path contains spaces, these need to be escaped like this: +path\ with\ spaces/ @space-owner +``` + +[introduced-mr]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/6916 +[ee]: https://about.gitlab.com/pricing/ diff --git a/doc/user/project/index.md b/doc/user/project/index.md index 1aeec6ec9d3e5c..7ff6718e0b5395 100644 --- a/doc/user/project/index.md +++ b/doc/user/project/index.md @@ -87,6 +87,8 @@ website with GitLab Pages your code blocks, overriding GitLab's default choice of language - [Badges](badges.md): Badges for the project overview - [Maven packages](maven_packages.md): Your private Maven repository in GitLab +- [Code owners](code_owners.md): Specify code owners for certain files + **[STARTER]** ### Project's integrations diff --git a/ee/app/helpers/ee/users_helper.rb b/ee/app/helpers/ee/users_helper.rb new file mode 100644 index 00000000000000..85ca4091e3a51b --- /dev/null +++ b/ee/app/helpers/ee/users_helper.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +module EE + module UsersHelper + def users_sentence(users, link_class: nil) + users.map { |user| link_to(user.name, user, class: link_class) }.to_sentence.html_safe + end + end +end diff --git a/ee/app/models/ee/blob.rb b/ee/app/models/ee/blob.rb new file mode 100644 index 00000000000000..6f3183a17ae520 --- /dev/null +++ b/ee/app/models/ee/blob.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module EE + module Blob + extend ActiveSupport::Concern + + def owners + @owners ||= ::Gitlab::CodeOwners.for_blob(self) + end + end +end diff --git a/ee/app/models/ee/repository.rb b/ee/app/models/ee/repository.rb index 6d712eca676bf5..ada64c4bcd2a62 100644 --- a/ee/app/models/ee/repository.rb +++ b/ee/app/models/ee/repository.rb @@ -87,5 +87,10 @@ def log_geo_updated_event def geo_updated_event_source is_wiki ? Geo::RepositoryUpdatedEvent::WIKI : Geo::RepositoryUpdatedEvent::REPOSITORY end + + def code_owners_blob(ref: 'HEAD') + possible_code_owner_blobs = ::Gitlab::CodeOwners::FILE_PATHS.map { |path| [ref, path] } + blobs_at(possible_code_owner_blobs).compact.first + end end end diff --git a/ee/app/models/license.rb b/ee/app/models/license.rb index 9c316a3fa79ecf..5d9a51248ba02a 100644 --- a/ee/app/models/license.rb +++ b/ee/app/models/license.rb @@ -9,6 +9,7 @@ class License < ActiveRecord::Base EES_FEATURES = %i[ audit_events burndown_charts + code_owners contribution_analytics elastic_search export_issues diff --git a/ee/app/views/projects/blob/_owners.html.haml b/ee/app/views/projects/blob/_owners.html.haml new file mode 100644 index 00000000000000..b86543a9d2e277 --- /dev/null +++ b/ee/app/views/projects/blob/_owners.html.haml @@ -0,0 +1,9 @@ +- return if blob.owners.empty? + +.well-segment.blob-auxiliary-viewer.file-owner-content + = sprite_icon('users', size: 18, css_class: 'icon') + %strong + = _("Code owners") + = link_to icon('question-circle'), help_page_path('user/project/code_owners'), title: 'About this feature', target: '_blank' + : + = users_sentence(blob.owners, link_class: 'file-owner-link') diff --git a/ee/changelogs/unreleased/bvl-codeowners-file.yml b/ee/changelogs/unreleased/bvl-codeowners-file.yml new file mode 100644 index 00000000000000..26229fb7b3330d --- /dev/null +++ b/ee/changelogs/unreleased/bvl-codeowners-file.yml @@ -0,0 +1,5 @@ +--- +title: Allow specifying code owners in a CODEOWNERS file +merge_request: 6916 +author: +type: added diff --git a/ee/lib/gitlab/code_owners.rb b/ee/lib/gitlab/code_owners.rb new file mode 100644 index 00000000000000..d7ae7dd856bae5 --- /dev/null +++ b/ee/lib/gitlab/code_owners.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module Gitlab + module CodeOwners + FILE_NAME = 'CODEOWNERS' + FILE_PATHS = [FILE_NAME, "docs/#{FILE_NAME}", ".gitlab/#{FILE_NAME}"].freeze + + def self.for_blob(blob) + if blob.project.feature_available?(:code_owners) + Loader.new(blob.project, blob.commit_id, blob.path).users + else + User.none + end + end + end +end diff --git a/ee/lib/gitlab/code_owners/file.rb b/ee/lib/gitlab/code_owners/file.rb new file mode 100644 index 00000000000000..f680c39727aea3 --- /dev/null +++ b/ee/lib/gitlab/code_owners/file.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +module Gitlab + module CodeOwners + class File + def initialize(blob) + @blob = blob + end + + def parsed_data + @parsed_data ||= get_parsed_data + end + + def empty? + parsed_data.empty? + end + + def owners_for_path(path) + matching_pattern = parsed_data.keys.reverse.detect do |pattern| + path_matches?(pattern, path) + end + + parsed_data[matching_pattern] + end + + private + + def data + if @blob && !@blob.binary? + @blob.data + else + "" + end + end + + def get_parsed_data + parsed = {} + + data.lines.each do |line| + line = line.strip + next unless line.present? + next if line.starts_with?('#') + + pattern, _separator, owners = line.partition(/(? Code owners', :js do + let(:project) { create(:project, :private, :repository) } + let(:user) { project.owner } + let(:code_owner) { create(:user, username: 'documentation-owner') } + + before do + sign_in(user) + project.add_developer(code_owner) + end + + def visit_blob(path, anchor: nil, ref: 'master') + visit project_blob_path(project, File.join(ref, path), anchor: anchor) + + wait_for_requests + end + + context 'when there is a codeowners file' do + context 'when the feature is available' do + before do + stub_licensed_features(code_owners: true) + end + + it 'shows the code owners related to a file' do + visit_blob('docs/CODEOWNERS', ref: 'with-codeowners') + + within('.file-owner-content') do + expect(page).to have_content('Code owners') + expect(page).to have_link(code_owner.name) + end + end + + it 'does not show the code owners banner when there are no code owners' do + visit_blob('README.md') + + expect(page).not_to have_content('Code owners:') + end + end + + context 'when the feature is not available' do + before do + stub_licensed_features(code_owners: false) + end + + it 'does not show the code owners related to a file' do + visit_blob('docs/CODEOWNERS', ref: 'with-codeowners') + + expect(page).not_to have_content('Code owners') + end + end + end +end diff --git a/ee/spec/fixtures/codeowners_example b/ee/spec/fixtures/codeowners_example new file mode 100644 index 00000000000000..d106d18b1dea73 --- /dev/null +++ b/ee/spec/fixtures/codeowners_example @@ -0,0 +1,44 @@ +# This is an example code owners file, lines starting with a `#` will +# be ignored. + +# app/ @commented-rule + +# We can specifiy a default match using wildcards: +* @default-codeowner + +# Rules defined later in the file take precedence over the rules +# defined before. +# This will match all files for which the file name ends in `.rb` +*.rb @ruby-owner + +# Files with a `#` can still be accesssed by escaping the pound sign +\#file_with_pound.rb @owner-file-with-pound + +# Multiple codeowners can be specified, separated by whitespace +CODEOWNERS @multiple @owners @tab-separated + +# Both usernames or email addresses can be used to match +# users. Everything else will be ignored. For example this will +# specify `@legal` and a user with email `janedoe@gitlab.com` as the +# owner for the LICENSE file +LICENSE @legal this does not match janedoe@gitlab.com + +# Ending a path in a `/` will specify the code owners for every file +# nested in that directory, on any level +/docs/ @all-docs + +# Ending a path in `/*` will specify code owners for every file in +# that directory, but not nested deeper. This will match +# `docs/index.md` but not `docs/projects/index.md` +/docs/* @root-docs + +# This will make a `lib` directory nested anywhere in the repository +# match +lib/ @lib-owner + +# This will only match a `config` directory in the root of the +# repository +/config/ @config-owner + +# If the path contains spaces, these need to be escaped like this: +path\ with\ spaces/ @space-owner diff --git a/ee/spec/lib/gitlab/code_owners/file_spec.rb b/ee/spec/lib/gitlab/code_owners/file_spec.rb new file mode 100644 index 00000000000000..355636a1021a69 --- /dev/null +++ b/ee/spec/lib/gitlab/code_owners/file_spec.rb @@ -0,0 +1,164 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::CodeOwners::File do + include FakeBlobHelpers + + let(:project) { build(:project) } + let(:file_content) do + File.read(Rails.root.join('ee', 'spec', 'fixtures', 'codeowners_example')) + end + + let(:blob) { fake_blob(path: 'CODEOWNERS', data: file_content) } + + subject(:file) { described_class.new(blob) } + + describe '#parsed_data' do + it 'parses all the required lines' do + expected_patterns = [ + '*', '**#file_with_pound.rb', '*.rb', '**CODEOWNERS', '**LICENSE', 'docs/', + 'docs/*', 'config/', '**lib/', '**path with spaces/' + ] + expect(file.parsed_data.keys) + .to contain_exactly(*expected_patterns) + end + + it 'allows usernames and emails' do + expect(file.parsed_data['**LICENSE']).to include('legal', 'janedoe@gitlab.com') + end + + context 'when there are entries that do not look like user references' do + let(:file_content) do + "a-path/ this is all random @username and email@gitlab.org" + end + + it 'ignores the entries' do + expect(file.parsed_data['**a-path/']).to include('username', 'email@gitlab.org') + end + end + end + + describe '#empty?' do + subject { file.empty? } + + it { is_expected.to be(false) } + + context 'when there is no content' do + let(:file_content) { "" } + + it { is_expected.to be(true) } + end + + context 'when the file is binary' do + let(:blob) { fake_blob(binary: true) } + + it { is_expected.to be(true) } + end + + context 'when the file did not exist' do + let(:blob) { nil } + + it { is_expected.to be(true) } + end + end + + describe '#owners_for_path' do + context 'for a path without matches' do + let(:file_content) do + <<~CONTENT + # Simulating a CODOWNERS without entries + CONTENT + end + + it 'returns an nil for an unmatched path' do + owners = file.owners_for_path('no_matches') + + expect(owners).to be_nil + end + end + + it 'matches random files to a pattern' do + owners = file.owners_for_path('app/assets/something.vue') + + expect(owners).to include('default-codeowner') + end + + it 'uses the last pattern if multiple patterns match' do + owners = file.owners_for_path('hello.rb') + + expect(owners).to eq('@ruby-owner') + end + + it 'returns the usernames for a file matching a pattern with a glob' do + owners = file.owners_for_path('app/models/repository.rb') + + expect(owners).to eq('@ruby-owner') + end + + it 'allows specifying multiple users' do + owners = file.owners_for_path('CODEOWNERS') + + expect(owners).to include('multiple', 'owners', 'tab-separated') + end + + it 'returns emails and usernames for a matched pattern' do + owners = file.owners_for_path('LICENSE') + + expect(owners).to include('legal', 'janedoe@gitlab.com') + end + + it 'allows escaping the pound sign used for comments' do + owners = file.owners_for_path('examples/#file_with_pound.rb') + + expect(owners).to include('owner-file-with-pound') + end + + it 'returns the usernames for a file nested in a directory' do + owners = file.owners_for_path('docs/projects/index.md') + + expect(owners).to include('all-docs') + end + + it 'returns the usernames for a pattern matched with a glob in a folder' do + owners = file.owners_for_path('docs/index.md') + + expect(owners).to include('root-docs') + end + + it 'allows matching files nested anywhere in the repository', :aggregate_failures do + lib_owners = file.owners_for_path('lib/gitlab/git/repository.rb') + other_lib_owners = file.owners_for_path('ee/lib/gitlab/git/repository.rb') + + expect(lib_owners).to include('lib-owner') + expect(other_lib_owners).to include('lib-owner') + end + + it 'allows allows limiting the matching files to the root of the repository', :aggregate_failures do + config_owners = file.owners_for_path('config/database.yml') + other_config_owners = file.owners_for_path('other/config/database.yml') + + expect(config_owners).to include('config-owner') + expect(other_config_owners).to eq('@default-codeowner') + end + + it 'correctly matches paths with spaces' do + owners = file.owners_for_path('path with spaces/README.md') + + expect(owners).to eq('@space-owner') + end + + context 'paths with whitespaces and username lookalikes' do + let(:file_content) do + 'a/weird\ path\ with/\ @username\ /\ and-email@lookalikes.com\ / @user-1 email@gitlab.org @user-2' + end + + it 'parses correctly' do + owners = file.owners_for_path('a/weird path with/ @username / and-email@lookalikes.com /test.rb') + + expect(owners).to include('user-1', 'user-2', 'email@gitlab.org') + expect(owners).not_to include('username', 'and-email@lookalikes.com') + end + end + end +end diff --git a/ee/spec/lib/gitlab/code_owners/loader_spec.rb b/ee/spec/lib/gitlab/code_owners/loader_spec.rb new file mode 100644 index 00000000000000..f2901f6836bb0c --- /dev/null +++ b/ee/spec/lib/gitlab/code_owners/loader_spec.rb @@ -0,0 +1,84 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::CodeOwners::Loader do + include FakeBlobHelpers + set(:group) { create(:group) } + set(:project) { create(:project, namespace: group) } + subject(:loader) { described_class.new(project, 'with-codeowners', path) } + + let!(:owner_1) { create(:user, username: 'owner-1') } + let!(:email_owner) { create(:user, username: 'owner-2') } + let!(:owner_3) { create(:user, username: 'owner-3') } + let!(:documentation_owner) { create(:user, username: 'documentation-owner') } + let(:codeowner_content) do + <<~CODEOWNERS + docs/* @documentation-owner + docs/CODEOWNERS @owner-1 owner2@gitlab.org @owner-3 @documentation-owner + CODEOWNERS + end + let(:codeowner_blob) { fake_blob(path: 'CODEOWNERS', data: codeowner_content) } + let(:path) { 'docs/CODEOWNERS' } + + before do + create(:email, user: email_owner, email: 'owner2@gitlab.org') + project.add_developer(owner_1) + project.add_developer(email_owner) + project.add_developer(documentation_owner) + + allow(project.repository).to receive(:code_owners_blob).and_return(codeowner_blob) + end + + describe '#users' do + context 'with a CODEOWNERS file' do + context 'for a path with code owners' do + it 'returns all existing users that are members of the project' do + expect(loader.users).to contain_exactly(owner_1, email_owner, documentation_owner) + end + + it 'does not return users that are not members of the project' do + expect(loader.users).not_to include(owner_3) + end + + it 'includes group members of the project' do + group.add_developer(owner_3) + + expect(loader.users).to include(owner_3) + end + end + + context 'for another path' do + let(:path) { 'no-codeowner' } + + it 'returns no users' do + expect(loader.users).to be_empty + end + end + end + + context 'when there is no codeowners file' do + let(:codeowner_blob) { nil } + + it 'returns no users without failing' do + expect(loader.users).to be_empty + end + end + + context 'with the request store', :request_store do + it 'only calls out to the repository once' do + expect(project.repository).to receive(:code_owners_blob).once + + 2.times { loader.users } + end + + it 'only processes the file once' do + code_owners_file = loader.__send__(:code_owners_file) + + expect(code_owners_file).to receive(:get_parsed_data).once.and_call_original + + 2.times { loader.users } + end + end + end +end diff --git a/ee/spec/lib/gitlab/code_owners_spec.rb b/ee/spec/lib/gitlab/code_owners_spec.rb new file mode 100644 index 00000000000000..49d86effe0b082 --- /dev/null +++ b/ee/spec/lib/gitlab/code_owners_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::CodeOwners do + include FakeBlobHelpers + + let!(:code_owner) { create(:user, username: 'owner-1') } + let(:project) { create(:project, :repository) } + let(:blob) do + project.repository.blob_at(TestEnv::BRANCH_SHA['with-codeowners'], 'docs/CODEOWNERS') + end + let(:codeowner_content) { "docs/CODEOWNERS @owner-1" } + let(:codeowner_blob) { fake_blob(path: 'CODEOWNERS', data: codeowner_content) } + + before do + project.add_developer(code_owner) + allow(project.repository).to receive(:code_owners_blob).and_return(codeowner_blob) + end + + describe '.for_blob' do + context 'when the feature is available' do + before do + stub_licensed_features(code_owners: true) + end + + it 'returns users for a blob' do + expect(described_class.for_blob(blob)).to include(code_owner) + end + end + + context 'when the feature is not available' do + before do + stub_licensed_features(code_owners: false) + end + + it 'returns no users' do + expect(described_class.for_blob(blob)).to be_empty + end + end + end +end diff --git a/ee/spec/models/blob_spec.rb b/ee/spec/models/blob_spec.rb new file mode 100644 index 00000000000000..2772e8a534c013 --- /dev/null +++ b/ee/spec/models/blob_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Blob do + let(:project) { create(:project, :repository) } + let(:blob) do + project.repository.blob_at(TestEnv::BRANCH_SHA['with-codeowners'], 'docs/CODEOWNERS') + end + let(:code_owner) { create(:user, username: 'documentation-owner') } + + before do + project.add_developer(code_owner) + end + + describe '#owners' do + context 'when the feature is available' do + before do + stub_licensed_features(code_owners: true) + end + + it 'returns the owners from the file' do + expect(blob.owners).to include(code_owner) + end + end + + context 'when the feature is not available' do + before do + stub_licensed_features(code_owners: false) + end + + it 'returns no code owners' do + expect(blob.owners).to be_empty + end + end + end +end diff --git a/ee/spec/models/repository_spec.rb b/ee/spec/models/repository_spec.rb index b397a5824be94b..df382e318adbb0 100644 --- a/ee/spec/models/repository_spec.rb +++ b/ee/spec/models/repository_spec.rb @@ -154,4 +154,24 @@ def keys_should_not_be_set end end end + + describe '#code_owners_blob' do + it 'returns nil if there is no codeowners file' do + expect(repository.code_owners_blob(ref: 'master')).to be_nil + end + + it 'returns the content of the codeowners file when it is found' do + expect(repository.code_owners_blob(ref: 'with-codeowners').data).to include('example CODEOWNERS file') + end + + it 'requests the CODOWNER blobs in batch in the correct order' do + expect(repository).to receive(:blobs_at) + .with([%w(HEAD CODEOWNERS), + %w(HEAD docs/CODEOWNERS), + %w(HEAD .gitlab/CODEOWNERS)]) + .and_call_original + + repository.code_owners_blob + end + end end diff --git a/lib/gitlab/user_extractor.rb b/lib/gitlab/user_extractor.rb new file mode 100644 index 00000000000000..3ede0a5b5e6b12 --- /dev/null +++ b/lib/gitlab/user_extractor.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +# This class extracts all users found in a piece of text by the username or the +# email adress + +module Gitlab + class UserExtractor + # Not using `Devise.email_regexp` to filter out any chars that an email + # does not end with and not pinning the email to a start of end of a string. + EMAIL_REGEXP = /(?([^@\s]+@[^@\s]+(? 'bar')).to eq(criteria) - end - end - - describe 'with multiple column/value pairs' do - it 'returns the criteria for a column and a value' do - initial = double(:criteria) - final = double(:criteria) - - expect(connection).to receive(:quote_table_name) - .with(:foo) - .and_return('"foo"') - - expect(connection).to receive(:quote_table_name) - .with(:bar) - .and_return('"bar"') - - expect(model).to receive(:where) - .with(%q{LOWER("foo") = LOWER(:value)}, value: 'bar') - .and_return(initial) - - expect(initial).to receive(:where) - .with(%q{LOWER("bar") = LOWER(:value)}, value: 'baz') - .and_return(final) - - got = model.iwhere(foo: 'bar', bar: 'baz') - - expect(got).to eq(final) - end - - it 'returns the criteria for a column with a table, and a value' do - initial = double(:criteria) - final = double(:criteria) - - expect(connection).to receive(:quote_table_name) - .with(:'foo.bar') - .and_return('"foo"."bar"') - - expect(connection).to receive(:quote_table_name) - .with(:'foo.baz') - .and_return('"foo"."baz"') - - expect(model).to receive(:where) - .with(%q{LOWER("foo"."bar") = LOWER(:value)}, value: 'bar') - .and_return(initial) - - expect(initial).to receive(:where) - .with(%q{LOWER("foo"."baz") = LOWER(:value)}, value: 'baz') - .and_return(final) - - got = model.iwhere('foo.bar'.to_sym => 'bar', - 'foo.baz'.to_sym => 'baz') - - expect(got).to eq(final) - end + let(:model) do + Class.new(ActiveRecord::Base) do + include CaseSensitivity + self.table_name = 'namespaces' end end - describe 'using MySQL' do - before do - allow(Gitlab::Database).to receive(:postgresql?).and_return(false) - allow(Gitlab::Database).to receive(:mysql?).and_return(true) - end - - describe 'with a single column/value pair' do - it 'returns the criteria for a column and a value' do - criteria = double(:criteria) - - expect(connection).to receive(:quote_table_name) - .with(:foo) - .and_return('`foo`') - - expect(model).to receive(:where) - .with(%q{`foo` = :value}, value: 'bar') - .and_return(criteria) + let!(:model_1) { model.create(path: 'mOdEl-1', name: 'mOdEl 1') } + let!(:model_2) { model.create(path: 'mOdEl-2', name: 'mOdEl 2') } - expect(model.iwhere(foo: 'bar')).to eq(criteria) - end + it 'finds a single instance by a single attribute regardless of case' do + expect(model.iwhere(path: 'MODEL-1')).to contain_exactly(model_1) + end - it 'returns the criteria for a column with a table, and a value' do - criteria = double(:criteria) + it 'finds multiple instances by a single attribute regardless of case' do + expect(model.iwhere(path: %w(MODEL-1 model-2))).to contain_exactly(model_1, model_2) + end - expect(connection).to receive(:quote_table_name) - .with(:'foo.bar') - .and_return('`foo`.`bar`') + it 'finds instances by multiple attributes' do + expect(model.iwhere(path: %w(MODEL-1 model-2), name: 'model 1')) + .to contain_exactly(model_1) + end - expect(model).to receive(:where) - .with(%q{`foo`.`bar` = :value}, value: 'bar') - .and_return(criteria) + # Using `mysql` & `postgresql` metadata-tags here because both adapters build + # the query slightly differently + context 'for MySQL', :mysql do + it 'builds a simple query' do + query = model.iwhere(path: %w(MODEL-1 model-2), name: 'model 1').to_sql + expected_query = <<~QRY.strip + SELECT `namespaces`.* FROM `namespaces` WHERE (`namespaces`.`path` IN ('MODEL-1', 'model-2')) AND (`namespaces`.`name` = 'model 1') + QRY - expect(model.iwhere('foo.bar'.to_sym => 'bar')) - .to eq(criteria) - end + expect(query).to eq(expected_query) end + end - describe 'with multiple column/value pairs' do - it 'returns the criteria for a column and a value' do - initial = double(:criteria) - final = double(:criteria) - - expect(connection).to receive(:quote_table_name) - .with(:foo) - .and_return('`foo`') - - expect(connection).to receive(:quote_table_name) - .with(:bar) - .and_return('`bar`') - - expect(model).to receive(:where) - .with(%q{`foo` = :value}, value: 'bar') - .and_return(initial) - - expect(initial).to receive(:where) - .with(%q{`bar` = :value}, value: 'baz') - .and_return(final) - - got = model.iwhere(foo: 'bar', bar: 'baz') - - expect(got).to eq(final) - end - - it 'returns the criteria for a column with a table, and a value' do - initial = double(:criteria) - final = double(:criteria) - - expect(connection).to receive(:quote_table_name) - .with(:'foo.bar') - .and_return('`foo`.`bar`') - - expect(connection).to receive(:quote_table_name) - .with(:'foo.baz') - .and_return('`foo`.`baz`') - - expect(model).to receive(:where) - .with(%q{`foo`.`bar` = :value}, value: 'bar') - .and_return(initial) - - expect(initial).to receive(:where) - .with(%q{`foo`.`baz` = :value}, value: 'baz') - .and_return(final) - - got = model.iwhere('foo.bar'.to_sym => 'bar', - 'foo.baz'.to_sym => 'baz') + context 'for PostgreSQL', :postgresql do + it 'builds a query using LOWER' do + query = model.iwhere(path: %w(MODEL-1 model-2), name: 'model 1').to_sql + expected_query = <<~QRY.strip + SELECT \"namespaces\".* FROM \"namespaces\" WHERE (LOWER(\"namespaces\".\"path\") IN (LOWER('MODEL-1'), LOWER('model-2'))) AND (LOWER(\"namespaces\".\"name\") = LOWER('model 1')) + QRY - expect(got).to eq(final) - end + expect(query).to eq(expected_query) end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index fd367d624ecbae..4f981cda2e6919 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -433,6 +433,23 @@ end end end + + describe '.by_username' do + it 'finds users regardless of the case passed' do + user = create(:user, username: 'CaMeLcAsEd') + user2 = create(:user, username: 'UPPERCASE') + + expect(described_class.by_username(%w(CAMELCASED uppercase))) + .to contain_exactly(user, user2) + end + + it 'finds a single user regardless of the case passed' do + user = create(:user, username: 'CaMeLcAsEd') + + expect(described_class.by_username('CAMELCASED')) + .to contain_exactly(user) + end + end end describe "Respond to" do diff --git a/spec/support/helpers/test_env.rb b/spec/support/helpers/test_env.rb index 21103771d1f742..3f8e3ae5190c57 100644 --- a/spec/support/helpers/test_env.rb +++ b/spec/support/helpers/test_env.rb @@ -52,7 +52,8 @@ module TestEnv 'add_images_and_changes' => '010d106', 'update-gitlab-shell-v-6-0-1' => '2f61d70', 'update-gitlab-shell-v-6-0-3' => 'de78448', - '2-mb-file' => 'bf12d25' + '2-mb-file' => 'bf12d25', + 'with-codeowners' => '219560e' }.freeze # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily -- GitLab From c06bc4e32a681d6a73877578fc836892feccea75 Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Tue, 4 Sep 2018 19:45:11 +0200 Subject: [PATCH 2/2] Refactor code owners docs --- doc/user/project/code_owners.md | 30 +++++++++++++++++------------- doc/user/project/index.md | 3 +-- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/doc/user/project/code_owners.md b/doc/user/project/code_owners.md index 8e80c1a21f306d..9d4e6cb8a78188 100644 --- a/doc/user/project/code_owners.md +++ b/doc/user/project/code_owners.md @@ -1,17 +1,25 @@ # Code owners -> [Introduced][introduced-mr] in [Gitlab Starter][ee] 11.3 +> [Introduced](https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/6916) +in [Gitlab Starter](https://about.gitlab.com/pricing/) 11.3. You can use a `CODEOWNERS` file to specify users that are responsible -for a certain part of the code in a repository. +for certain files in a repository. -The codeowners file can be added to the root of the repository, inside -the `.gitlab/` or the `docs/` folder. +You can choose and add the `CODEOWNERS` file in three places: -The `CODEOWNERS` is scoped to a branch. This means that with the -introduction of new files, the developer adding the new code can -specify themselves as code owner. Before the new code gets merged to -master. +- to the root directory of the repository +- inside the `.gitlab/` directory +- inside the `docs/` directory + +The `CODEOWNERS` file is scoped to a branch, which means that with the +introduction of new files, the person adding the new content can +specify themselves as a code owner, all before the new changes +get merged to the default branch. + +When a file matches multiple entries in the `CODEOWNERS` file, +the users from all entries are displayed on the blob page of +the given file. ## The syntax of a code owners file @@ -53,7 +61,7 @@ CODEOWNERS @multiple @owners @tab-separated # users. Everything else will be ignored. For example this will # specify `@legal` and a user with email `janedoe@gitlab.com` as the # owner for the LICENSE file -LICENSE @legal this does not match janedoe@gitlab.com +LICENSE @legal this_does_not_match janedoe@gitlab.com # Ending a path in a `/` will specify the code owners for every file # nested in that directory, on any level @@ -72,10 +80,6 @@ lib/ @lib-owner # repository /config/ @config-owner - # If the path contains spaces, these need to be escaped like this: path\ with\ spaces/ @space-owner ``` - -[introduced-mr]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/6916 -[ee]: https://about.gitlab.com/pricing/ diff --git a/doc/user/project/index.md b/doc/user/project/index.md index 7ff6718e0b5395..e1677218447db4 100644 --- a/doc/user/project/index.md +++ b/doc/user/project/index.md @@ -87,8 +87,7 @@ website with GitLab Pages your code blocks, overriding GitLab's default choice of language - [Badges](badges.md): Badges for the project overview - [Maven packages](maven_packages.md): Your private Maven repository in GitLab -- [Code owners](code_owners.md): Specify code owners for certain files - **[STARTER]** +- [Code owners](code_owners.md): Specify code owners for certain files **[STARTER]** ### Project's integrations -- GitLab