diff options
| -rw-r--r-- | app/assets/javascripts/error_tracking/components/error_details.vue | 6 | ||||
| -rw-r--r-- | app/controllers/projects/issues_controller.rb | 5 | ||||
| -rw-r--r-- | app/models/issue.rb | 2 | ||||
| -rw-r--r-- | app/models/sentry_issue.rb | 4 | ||||
| -rw-r--r-- | doc/development/dangerbot.md | 6 | ||||
| -rw-r--r-- | doc/user/project/clusters/serverless/index.md | 4 | ||||
| -rw-r--r-- | lib/api/entities.rb | 6 | ||||
| -rw-r--r-- | lib/api/groups.rb | 2 | ||||
| -rw-r--r-- | lib/api/projects.rb | 34 | ||||
| -rw-r--r-- | lib/api/projects_batch_counting.rb | 27 | ||||
| -rw-r--r-- | lib/api/projects_relation_builder.rb | 36 | ||||
| -rw-r--r-- | spec/controllers/projects/issues_controller_spec.rb | 18 | ||||
| -rw-r--r-- | spec/frontend/error_tracking/components/error_details_spec.js | 9 | ||||
| -rw-r--r-- | spec/lib/api/projects_batch_counting_spec.rb | 72 | ||||
| -rw-r--r-- | spec/models/sentry_issue_spec.rb | 1 | ||||
| -rw-r--r-- | spec/requests/api/projects_spec.rb | 110 |
16 files changed, 98 insertions, 244 deletions
diff --git a/app/assets/javascripts/error_tracking/components/error_details.vue b/app/assets/javascripts/error_tracking/components/error_details.vue index 9cf141589db..14b2e59009a 100644 --- a/app/assets/javascripts/error_tracking/components/error_details.vue +++ b/app/assets/javascripts/error_tracking/components/error_details.vue @@ -118,13 +118,17 @@ export default { <div v-if="loading" class="py-3"> <gl-loading-icon :size="3" /> </div> - <div v-else-if="showDetails" class="error-details"> <div class="top-area align-items-center justify-content-between py-3"> <span v-if="!loadingStacktrace && stacktrace" v-html="reported"></span> <form ref="sentryIssueForm" :action="projectIssuesPath" method="POST"> <gl-form-input class="hidden" name="issue[title]" :value="issueTitle" /> <input name="issue[description]" :value="issueDescription" type="hidden" /> + <gl-form-input + :value="error.id" + class="hidden" + name="issue[sentry_issue_attributes][sentry_issue_identifier]" + /> <gl-form-input :value="csrfToken" class="hidden" name="authenticity_token" /> <loading-button v-if="!error.gitlab_issue" diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 5cbfabebe39..229374c3929 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -237,7 +237,10 @@ class Projects::IssuesController < Projects::ApplicationController end def issue_params - params.require(:issue).permit(*issue_params_attributes) + params.require(:issue).permit( + *issue_params_attributes, + sentry_issue_attributes: [:sentry_issue_identifier] + ) end def issue_params_attributes diff --git a/app/models/issue.rb b/app/models/issue.rb index d49f3c28000..f16fd8e63ec 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -45,6 +45,8 @@ class Issue < ApplicationRecord has_many :user_mentions, class_name: "IssueUserMention" has_one :sentry_issue + accepts_nested_attributes_for :sentry_issue + validates :project, presence: true alias_attribute :parent_ids, :project_id diff --git a/app/models/sentry_issue.rb b/app/models/sentry_issue.rb index 678e09b5370..6be52f99562 100644 --- a/app/models/sentry_issue.rb +++ b/app/models/sentry_issue.rb @@ -4,5 +4,7 @@ class SentryIssue < ApplicationRecord belongs_to :issue validates :issue, uniqueness: true, presence: true - validates :sentry_issue_identifier, presence: true + validates :sentry_issue_identifier, + uniqueness: true, + presence: true end diff --git a/doc/development/dangerbot.md b/doc/development/dangerbot.md index 0b2e2b43512..40eb4294617 100644 --- a/doc/development/dangerbot.md +++ b/doc/development/dangerbot.md @@ -95,6 +95,12 @@ through in CI. However, you can speed these cycles up somewhat by emptying the `.gitlab/ci/rails.gitlab-ci.yml` file in your merge request. Just don't forget to revert the change before merging! +To enable the Dangerfile on another existing GitLab project, run the following extra steps, based on [this procedure](https://danger.systems/guides/getting_started.html#creating-a-bot-account-for-danger-to-use): + +1. Add `@gitlab-bot` to the project as a `reporter`. +1. Add the `@gitlab-bot`'s `GITLAB_API_PRIVATE_TOKEN` value as a value for a new CI/CD + variable named `DANGER_GITLAB_API_TOKEN`. + You should add the `~Danger bot` label to the merge request before sending it for review. diff --git a/doc/user/project/clusters/serverless/index.md b/doc/user/project/clusters/serverless/index.md index 55bce10a49d..81aff05a32a 100644 --- a/doc/user/project/clusters/serverless/index.md +++ b/doc/user/project/clusters/serverless/index.md @@ -13,7 +13,7 @@ GitLab supports several ways deploy Serverless applications in both Kubernetes E Currently we support: -- [Knative](#knative): Build Knative applications with Knative and `gitlabktl` on GKE. +- [Knative](#knative): Build Knative applications with Knative and `gitlabktl` on GKE and EKS. - [AWS Lambda](aws.md): Create serverless applications via the Serverless Framework and GitLab CI. ## Knative @@ -89,7 +89,7 @@ The minimum recommended cluster size to run Knative is 3-nodes, 6 vCPUs, and 22. for other platforms [Install kubectl](https://kubernetes.io/docs/tasks/tools/install-kubectl/). 1. The Ingress is now available at this address and will route incoming requests to the proper service based on the DNS - name in the request. To support this, a wildcard DNS A record should be created for the desired domain name. For example, + name in the request. To support this, a wildcard DNS record should be created for the desired domain name. For example, if your Knative base domain is `knative.info` then you need to create an A record or CNAME record with domain `*.knative.info` pointing the ip address or hostname of the Ingress. diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 8a60127ff59..6d33cb214ee 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -188,7 +188,7 @@ module API end class BasicProjectDetails < ProjectIdentity - include ::API::ProjectsBatchCounting + include ::API::ProjectsRelationBuilder expose :default_branch, if: -> (project, options) { Ability.allowed?(options[:current_user], :download_code, project) } # Avoids an N+1 query: https://github.com/mbleigh/acts-as-taggable-on/issues/91#issuecomment-168273770 @@ -430,7 +430,7 @@ module API options: { only_owned: true, limit: projects_limit } ).execute - Entities::Project.preload_and_batch_count!(projects) + Entities::Project.prepare_relation(projects) end expose :shared_projects, using: Entities::Project do |group, options| @@ -440,7 +440,7 @@ module API options: { only_shared: true, limit: projects_limit } ).execute - Entities::Project.preload_and_batch_count!(projects) + Entities::Project.prepare_relation(projects) end def projects_limit diff --git a/lib/api/groups.rb b/lib/api/groups.rb index b9cfc16fb23..6c88b61eee8 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -231,7 +231,7 @@ module API projects, options = with_custom_attributes(projects, options) - present options[:with].preload_and_batch_count!(projects), options + present options[:with].prepare_relation(projects), options end desc 'Get a list of subgroups in this group.' do diff --git a/lib/api/projects.rb b/lib/api/projects.rb index ea7087d2f46..a1fce9e8b20 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -75,17 +75,15 @@ module API mutually_exclusive :import_url, :template_name, :template_project_id end - def find_projects + def load_projects ProjectsFinder.new(current_user: current_user, params: project_finder_params).execute end - # Prepare the full projects query - # None of this is supposed to actually execute any database query - def prepare_query(projects) + def present_projects(projects, options = {}) projects = reorder_projects(projects) projects = apply_filters(projects) - - projects, options = with_custom_attributes(projects) + projects = paginate(projects) + projects, options = with_custom_attributes(projects, options) options = options.reverse_merge( with: current_user ? Entities::ProjectWithAccess : Entities::BasicProjectDetails, @@ -93,23 +91,9 @@ module API current_user: current_user, license: false ) - options[:with] = Entities::BasicProjectDetails if params[:simple] - projects = options[:with].preload_relation(projects, options) - - [projects, options] - end - - def prepare_and_present(project_relation) - projects, options = prepare_query(project_relation) - - projects = paginate_and_retrieve!(projects) - - # Refresh count caches - options[:with].execute_batch_counting(projects) - - present projects, options + present options[:with].prepare_relation(projects, options), options end def translate_params_for_compatibility(params) @@ -134,7 +118,7 @@ module API params[:user] = user - prepare_and_present find_projects + present_projects load_projects end desc 'Get projects starred by a user' do @@ -150,7 +134,7 @@ module API not_found!('User') unless user starred_projects = StarredProjectsFinder.new(user, params: project_finder_params, current_user: current_user).execute - prepare_and_present starred_projects + present_projects starred_projects end end @@ -166,7 +150,7 @@ module API use :with_custom_attributes end get do - prepare_and_present find_projects + present_projects load_projects end desc 'Create new project' do @@ -303,7 +287,7 @@ module API get ':id/forks' do forks = ForkProjectsFinder.new(user_project, params: project_finder_params, current_user: current_user).execute - prepare_and_present forks + present_projects forks end desc 'Check pages access of this project' diff --git a/lib/api/projects_batch_counting.rb b/lib/api/projects_batch_counting.rb deleted file mode 100644 index 4e5124c8791..00000000000 --- a/lib/api/projects_batch_counting.rb +++ /dev/null @@ -1,27 +0,0 @@ -# frozen_string_literal: true - -module API - module ProjectsBatchCounting - extend ActiveSupport::Concern - - class_methods do - # This adds preloading to the query and executes batch counting - # Side-effect: The query will be executed during batch counting - def preload_and_batch_count!(projects_relation) - preload_relation(projects_relation).tap do |projects| - execute_batch_counting(projects) - end - end - - def execute_batch_counting(projects) - ::Projects::BatchForksCountService.new(forks_counting_projects(projects)).refresh_cache - - ::Projects::BatchOpenIssuesCountService.new(projects).refresh_cache - end - - def forks_counting_projects(projects) - projects - end - end - end -end diff --git a/lib/api/projects_relation_builder.rb b/lib/api/projects_relation_builder.rb new file mode 100644 index 00000000000..263468c9aa6 --- /dev/null +++ b/lib/api/projects_relation_builder.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module API + module ProjectsRelationBuilder + extend ActiveSupport::Concern + + class_methods do + def prepare_relation(projects_relation, options = {}) + projects_relation = preload_relation(projects_relation, options) + execute_batch_counting(projects_relation) + projects_relation + end + + def preload_relation(projects_relation, options = {}) + projects_relation + end + + def forks_counting_projects(projects_relation) + projects_relation + end + + def batch_forks_counting(projects_relation) + ::Projects::BatchForksCountService.new(forks_counting_projects(projects_relation)).refresh_cache + end + + def batch_open_issues_counting(projects_relation) + ::Projects::BatchOpenIssuesCountService.new(projects_relation).refresh_cache + end + + def execute_batch_counting(projects_relation) + batch_forks_counting(projects_relation) + batch_open_issues_counting(projects_relation) + end + end + end +end diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 51d5a0d2144..e4e2487f1e1 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -1073,6 +1073,24 @@ describe Projects::IssuesController do expect(issue.time_estimate).to eq(7200) end end + + context 'when created from sentry error' do + subject { post_new_issue(sentry_issue_attributes: { sentry_issue_identifier: 1234567 }) } + + it 'creates an issue' do + expect { subject }.to change(Issue, :count) + end + + it 'creates a sentry issue' do + expect { subject }.to change(SentryIssue, :count) + end + + it 'with existing issue it will not create an issue' do + post_new_issue(sentry_issue_attributes: { sentry_issue_identifier: 1234567 }) + + expect { subject }.not_to change(Issue, :count) + end + end end describe 'POST #mark_as_spam' do diff --git a/spec/frontend/error_tracking/components/error_details_spec.js b/spec/frontend/error_tracking/components/error_details_spec.js index 632ed373545..6dc4980aaec 100644 --- a/spec/frontend/error_tracking/components/error_details_spec.js +++ b/spec/frontend/error_tracking/components/error_details_spec.js @@ -110,7 +110,7 @@ describe('ErrorDetails', () => { beforeEach(() => { store.state.details.loading = false; store.state.details.error = { - id: 1, + id: 129381, title: 'Issue title', external_url: 'http://sentry.gitlab.net/gitlab', first_seen: '2017-05-26T13:32:48Z', @@ -121,6 +121,13 @@ describe('ErrorDetails', () => { mountComponent(); }); + it('should send sentry_issue_identifier', () => { + const sentryErrorIdInput = wrapper.find( + 'glforminput-stub[name="issue[sentry_issue_attributes][sentry_issue_identifier]"', + ); + expect(sentryErrorIdInput.attributes('value')).toBe('129381'); + }); + it('should set the form values with title and description', () => { const csrfTokenInput = wrapper.find('glforminput-stub[name="authenticity_token"]'); const issueTitleInput = wrapper.find('glforminput-stub[name="issue[title]"]'); diff --git a/spec/lib/api/projects_batch_counting_spec.rb b/spec/lib/api/projects_batch_counting_spec.rb deleted file mode 100644 index 6094952bb52..00000000000 --- a/spec/lib/api/projects_batch_counting_spec.rb +++ /dev/null @@ -1,72 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe API::ProjectsBatchCounting do - subject do - Class.new do - include ::API::ProjectsBatchCounting - end - end - - describe '.preload_and_batch_count!' do - let(:projects) { double } - let(:preloaded_projects) { double } - - it 'preloads the relation' do - allow(subject).to receive(:execute_batch_counting).with(preloaded_projects) - - expect(subject).to receive(:preload_relation).with(projects).and_return(preloaded_projects) - - expect(subject.preload_and_batch_count!(projects)).to eq(preloaded_projects) - end - - it 'executes batch counting' do - allow(subject).to receive(:preload_relation).with(projects).and_return(preloaded_projects) - - expect(subject).to receive(:execute_batch_counting).with(preloaded_projects) - - subject.preload_and_batch_count!(projects) - end - end - - describe '.execute_batch_counting' do - let(:projects) { create_list(:project, 2) } - let(:count_service) { double } - - it 'counts forks' do - allow(::Projects::BatchForksCountService).to receive(:new).with(projects).and_return(count_service) - - expect(count_service).to receive(:refresh_cache) - - subject.execute_batch_counting(projects) - end - - it 'counts open issues' do - allow(::Projects::BatchOpenIssuesCountService).to receive(:new).with(projects).and_return(count_service) - - expect(count_service).to receive(:refresh_cache) - - subject.execute_batch_counting(projects) - end - - context 'custom fork counting' do - subject do - Class.new do - include ::API::ProjectsBatchCounting - def self.forks_counting_projects(projects) - [projects.first] - end - end - end - - it 'counts forks for other projects' do - allow(::Projects::BatchForksCountService).to receive(:new).with([projects.first]).and_return(count_service) - - expect(count_service).to receive(:refresh_cache) - - subject.execute_batch_counting(projects) - end - end - end -end diff --git a/spec/models/sentry_issue_spec.rb b/spec/models/sentry_issue_spec.rb index b3f7d2628db..48f9adf64af 100644 --- a/spec/models/sentry_issue_spec.rb +++ b/spec/models/sentry_issue_spec.rb @@ -13,5 +13,6 @@ describe SentryIssue do it { is_expected.to validate_presence_of(:issue) } it { is_expected.to validate_uniqueness_of(:issue) } it { is_expected.to validate_presence_of(:sentry_issue_identifier) } + it { is_expected.to validate_uniqueness_of(:sentry_issue_identifier).with_message("has already been taken") } end end diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 5161de6de5e..cda2dd7d2f4 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -155,35 +155,6 @@ describe API::Projects do project4 end - # This is a regression spec for https://gitlab.com/gitlab-org/gitlab/issues/37919 - context 'batch counting forks and open issues and refreshing count caches' do - # We expect to count these projects (only the ones on the first page, not all matching ones) - let(:projects) { Project.public_to_user(nil).order(id: :desc).first(per_page) } - let(:per_page) { 2 } - let(:count_service) { double } - - before do - # Create more projects, so we have more than one page - create_list(:project, 5, :public) - end - - it 'batch counts project forks' do - expect(::Projects::BatchForksCountService).to receive(:new).with(projects).and_return(count_service) - expect(count_service).to receive(:refresh_cache) - - get api("/projects?per_page=#{per_page}") - expect(response.status).to eq 200 - end - - it 'batch counts open issues' do - expect(::Projects::BatchOpenIssuesCountService).to receive(:new).with(projects).and_return(count_service) - expect(count_service).to receive(:refresh_cache) - - get api("/projects?per_page=#{per_page}") - expect(response.status).to eq 200 - end - end - context 'when unauthenticated' do it_behaves_like 'projects response' do let(:filter) { { search: project.name } } @@ -599,87 +570,6 @@ describe API::Projects do let(:projects) { Project.all } end end - - context 'with keyset pagination' do - let(:current_user) { user } - let(:projects) { [public_project, project, project2, project3] } - - context 'headers and records' do - let(:params) { { pagination: 'keyset', order_by: :id, sort: :asc, per_page: 1 } } - - it 'includes a pagination header with link to the next page' do - get api('/projects', current_user), params: params - - expect(response.header).to include('Links') - expect(response.header['Links']).to include('pagination=keyset') - expect(response.header['Links']).to include("id_after=#{public_project.id}") - end - - it 'contains only the first project with per_page = 1' do - get api('/projects', current_user), params: params - - expect(response).to have_gitlab_http_status(200) - expect(json_response).to be_an Array - expect(json_response.map { |p| p['id'] }).to contain_exactly(public_project.id) - end - - it 'does not include a link if the end has reached and there is no more data' do - get api('/projects', current_user), params: params.merge(id_after: project2.id) - - expect(response.header).not_to include('Links') - end - - it 'responds with 501 if order_by is different from id' do - get api('/projects', current_user), params: params.merge(order_by: :created_at) - - expect(response).to have_gitlab_http_status(405) - end - end - - context 'with descending sorting' do - let(:params) { { pagination: 'keyset', order_by: :id, sort: :desc, per_page: 1 } } - - it 'includes a pagination header with link to the next page' do - get api('/projects', current_user), params: params - - expect(response.header).to include('Links') - expect(response.header['Links']).to include('pagination=keyset') - expect(response.header['Links']).to include("id_before=#{project3.id}") - end - - it 'contains only the last project with per_page = 1' do - get api('/projects', current_user), params: params - - expect(response).to have_gitlab_http_status(200) - expect(json_response).to be_an Array - expect(json_response.map { |p| p['id'] }).to contain_exactly(project3.id) - end - end - - context 'retrieving the full relation' do - let(:params) { { pagination: 'keyset', order_by: :id, sort: :desc, per_page: 2 } } - - it 'returns all projects' do - url = '/projects' - requests = 0 - ids = [] - - while url && requests <= 5 # circuit breaker - requests += 1 - get api(url, current_user), params: params - - links = response.header['Links'] - url = links&.match(/<[^>]+(\/projects\?[^>]+)>; rel="next"/) do |match| - match[1] - end - - ids += JSON.parse(response.body).map { |p| p['id'] } - end - - expect(ids).to contain_exactly(*projects.map(&:id)) - end - end - end end describe 'POST /projects' do |