diff options
29 files changed, 692 insertions, 73 deletions
diff --git a/app/controllers/projects/settings/ci_cd_controller.rb b/app/controllers/projects/settings/ci_cd_controller.rb index 1c3008181b2..a545c2e5b3c 100644 --- a/app/controllers/projects/settings/ci_cd_controller.rb +++ b/app/controllers/projects/settings/ci_cd_controller.rb @@ -123,10 +123,12 @@ module Projects .with_tags active_shared_runners = ::Ci::Runner.instance_type.active - @shared_runners_count = active_shared_runners.count(:all) + @shared_runners_count = active_shared_runners.count @shared_runners = active_shared_runners.page(params[:shared_runners_page]).per(NUMBER_OF_RUNNERS_PER_PAGE).with_tags - @group_runners = ::Ci::Runner.belonging_to_parent_group_of_project(@project.id).with_tags + parent_group_runners = ::Ci::Runner.belonging_to_parent_group_of_project(@project.id) + @group_runners_count = parent_group_runners.count + @group_runners = parent_group_runners.page(params[:group_runners_page]).per(NUMBER_OF_RUNNERS_PER_PAGE).with_tags end def define_ci_variables diff --git a/app/helpers/groups/settings_helper.rb b/app/helpers/groups/settings_helper.rb index 1b391680996..38300043dd7 100644 --- a/app/helpers/groups/settings_helper.rb +++ b/app/helpers/groups/settings_helper.rb @@ -9,7 +9,7 @@ module Groups remove_form_id: remove_form_id, button_text: _('Remove group'), button_testid: 'remove-group-button', - disabled: group.paid?.to_s, + disabled: group.prevent_delete?.to_s, confirm_danger_message: remove_group_message(group), phrase: group.full_path } diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 98a9ccc2040..08c56131d78 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -414,6 +414,29 @@ class MergeRequestDiff < ApplicationRecord end end + def paginated_diffs(page, per_page) + fetching_repository_diffs({}) do |comparison| + reorder_diff_files! + + collection = Gitlab::Diff::FileCollection::PaginatedMergeRequestDiff.new( + self, + page, + per_page + ) + + if comparison + comparison.diffs( + paths: collection.diff_paths, + page: collection.current_page, + per_page: collection.limit_value, + count: collection.total_count + ) + else + collection + end + end + end + def diffs(diff_options = nil) fetching_repository_diffs(diff_options) do |comparison| # It should fetch the repository when diffs are cleaned by the system. diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 51c39ad4ec3..47fd39e8b96 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -507,6 +507,10 @@ class Namespace < ApplicationRecord root? && actual_plan.paid? end + def prevent_delete? + paid? + end + def actual_limits # We default to PlanLimits.new otherwise a lot of specs would fail # On production each plan should already have associated limits record diff --git a/app/views/groups/settings/_remove_button.html.haml b/app/views/groups/settings/_remove_button.html.haml index 3268352723e..cb05076b39d 100644 --- a/app/views/groups/settings/_remove_button.html.haml +++ b/app/views/groups/settings/_remove_button.html.haml @@ -1,6 +1,6 @@ - remove_form_id = local_assigns.fetch(:remove_form_id, nil) -- if group.paid? +- if group.prevent_delete? = render Pajamas::AlertComponent.new(dismissible: false, alert_options: { class: 'gl-mb-5', data: { testid: 'group-has-linked-subscription-alert' }}) do |c| = c.body do = html_escape(_("This group can't be removed because it is linked to a subscription. To remove this group, %{linkStart}link the subscription%{linkEnd} with a different group.")) % { linkStart: "<a href=\"#{help_page_path('subscriptions/gitlab_com/index', anchor: 'change-the-linked-namespace')}\">".html_safe, linkEnd: '</a>'.html_safe } diff --git a/app/views/projects/runners/_group_runners.html.haml b/app/views/projects/runners/_group_runners.html.haml index 5acd6f95df4..d71bcd12e64 100644 --- a/app/views/projects/runners/_group_runners.html.haml +++ b/app/views/projects/runners/_group_runners.html.haml @@ -35,7 +35,9 @@ = _('Ask your group owner to set up a group runner.') - else - %h4.underlined-title - = _('Available group runners: %{runners}').html_safe % { runners: @group_runners.count } - %ul.bordered-list - = render partial: 'projects/runners/runner', collection: @group_runners, as: :runner + %div{ data: { testid: 'group-runners' } } + %h5.gl-mt-6.gl-mb-0 + = _('Available group runners: %{runners}') % { runners: @group_runners_count } + %ul.bordered-list + = render partial: 'projects/runners/runner', collection: @group_runners, as: :runner + = paginate @group_runners, theme: "gitlab", param_name: "group_runners_page", params: { expand_runners: true, anchor: 'js-runners-settings' } diff --git a/app/views/projects/runners/_shared_runners.html.haml b/app/views/projects/runners/_shared_runners.html.haml index 8b18a6a9c05..9e7bbd6cefe 100644 --- a/app/views/projects/runners/_shared_runners.html.haml +++ b/app/views/projects/runners/_shared_runners.html.haml @@ -5,8 +5,9 @@ - if @shared_runners_count == 0 = _('This GitLab instance does not provide any shared runners yet. Instance administrators can register shared runners in the admin area.') - else - %h5.gl-mt-6.gl-mb-0 #{_('Available shared runners:')} #{@shared_runners_count} %div{ data: { testid: 'available-shared-runners' } } + %h5.gl-mt-6.gl-mb-0 + = s_('Runners|Available shared runners: %{count}') % {count: @shared_runners_count} %ul.bordered-list = render partial: 'projects/runners/runner', collection: @shared_runners, as: :runner = paginate @shared_runners, theme: "gitlab", param_name: "shared_runners_page", params: { expand_runners: true, anchor: 'js-runners-settings' } diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 28233efdcd8..6ee74cc73ce 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -1101,6 +1101,74 @@ Supported attributes: } ``` +WARNING: +This endpoint was [deprecated](https://gitlab.com/gitlab-org/gitlab/-/issues/322117) in GitLab 15.7 +and will be removed in API v5. Use the [List merge request diffs](#list-merge-request-diffs) endpoint instead. + +## List merge request diffs + +List diffs of the files changed in a merge request. + +```plaintext +GET /projects/:id/merge_requests/:merge_request_iid/diffs +``` + +Supported attributes: + +| Attribute | Type | Required | Description | +|-----------|------|----------|-------------| +| `id` | integer or string | **{check-circle}** Yes | The ID or [URL-encoded path of the project](index.md#namespaced-path-encoding) owned by the authenticated user. | +| `merge_request_iid` | integer | **{check-circle}** Yes | The internal ID of the merge request. | +| `page` | integer | **{dotted-circle}** no | The page of results to return. Defaults to 1. | +| `per_page` | integer | **{dotted-circle}** no | The number of results per page. Defaults to 20. | + +If successful, returns [`200 OK`](index.md#status-codes) and the +following response attributes: + +| Attribute | Type | Description | +|:----------|:-----|:------------| +| `old_path` | string | Old path of the file. | +| `new_path` | string | New path of the file. | +| `a_mode` | string | Old file mode of the file. | +| `b_mode` | string | New file mode of the file. | +| `diff` | string | Diff representation of the changes made on the file. | +| `new_file` | boolean | Indicates if the file has just been added. | +| `renamed_file` | boolean | Indicates if the file has been renamed. | +| `deleted_file` | boolean | Indicates if the file has been removed. | + +Example request: + +```shell +curl --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/projects/1/merge_requests/1/diffs?page=1&per_page=2" +``` + +Example response: + +```json +[ + { + "old_path": "README", + "new_path": "README", + "a_mode": "100644", + "b_mode": "100644", + "diff": "--- a/README\ +++ b/README\ @@ -1 +1 @@\ -Title\ +README", + "new_file": false, + "renamed_file": false, + "deleted_file": false + }, + { + "old_path": "VERSION", + "new_path": "VERSION", + "a_mode": "100644", + "b_mode": "100644", + "diff": "--- a/VERSION\ +++ b/VERSION\ @@ -1 +1 @@\ -1.9.7\ +1.9.8", + "new_file": false, + "renamed_file": false, + "deleted_file": false + } +] +``` + ## List merge request pipelines Get a list of merge request pipelines. diff --git a/doc/architecture/blueprints/pods/pods-feature-container-registry.md b/doc/architecture/blueprints/pods/pods-feature-container-registry.md index 93aed00bd61..4a2e885a8c7 100644 --- a/doc/architecture/blueprints/pods/pods-feature-container-registry.md +++ b/doc/architecture/blueprints/pods/pods-feature-container-registry.md @@ -14,16 +14,118 @@ we can document the reasons for not choosing this approach. # Pods: Container Registry -> TL;DR +GitLab Container Registry is a feature allowing to store Docker Container Images +in GitLab. You can read about GitLab integration [here](../../../user/packages/container_registry/index.md). ## 1. Definition +GitLab Container Registry is a complex service requiring usage of PostgreSQL, Redis +and Object Storage dependencies. Right now there's undergoing work to introduce +[Container Registry Metadata](../container_registry_metadata_database/index.md) +to optimize data storage and image retention policies of Container Registry. + +GitLab Container Registry is serving as a container for stored data, +but on it's own does not authenticate `docker login`. The `docker login` +is executed with user credentials (can be `personal access token`) +or CI build credentials (ephemeral `ci_builds.token`). + +Container Registry uses data deduplication. It means that the same blob +(image layer) that is shared between many projects is stored only once. +Each layer is hashed by `sha256`. + +The `docker login` does request JWT time-limited authentication token that +is signed by GitLab, but validated by Container Registry service. The JWT +token does store all authorized scopes (`container repository images`) +and operation types (`push` or `pull`). A single JWT authentication token +can be have many authorized scopes. This allows container registry and client +to mount existing blobs from another scopes. GitLab responds only with +authorized scopes. Then it is up to GitLab Container Registry to validate +if the given operation can be performed. + +The GitLab.com pages are always scoped to project. Each project can have many +container registry images attached. + +Currently in case of GitLab.com the actual registry service is served +via `https://registry.gitlab.com`. + +The main identifiable problems are: + +- the authentication reqest (`https://gitlab.com/jwt/auth`) that is processed by GitLab.com +- the `https://registry.gitlab.com` that is run by external service and uses it's own data store +- the data deduplication, the Pods architecture with registry run in a Pod would reduce + efficiency of data storage + ## 2. Data flow +### 2.1. Authorization request that is send by `docker login` + +```shell +curl \ + --user "username:password" \ + "https://gitlab/jwt/auth?client_id=docker&offline_token=true&service=container_registry&scope=repository:gitlab-org/gitlab-build-images:push,pull" +``` + +Result is encoded and signed JWT token. Second base64 encoded string (split by `.`) contains JSON with authorized scopes. + +```json +{"auth_type":"none","access":[{"type":"repository","name":"gitlab-org/gitlab-build-images","actions":["pull"]}],"jti":"61ca2459-091c-4496-a3cf-01bac51d4dc8","aud":"container_registry","iss":"omnibus-gitlab-issuer","iat":1669309469,"nbf":166} +``` + +### 2.2. Docker client fetching tags + +```shell +curl \ + -H "Accept: application/vnd.docker.distribution.manifest.v2+json" \ + -H "Authorization: Bearer token" \ + https://registry.gitlab.com/v2/gitlab-org/gitlab-build-images/tags/list + +curl \ + -H "Accept: application/vnd.docker.distribution.manifest.v2+json" \ + -H "Authorization: Bearer token" \ + https://registry.gitlab.com/v2/gitlab-org/gitlab-build-images/manifests/danger-ruby-2.6.6 +``` + +### 2.3. Docker client fetching blobs and manifests + +```shell +curl \ + -H "Accept: application/vnd.docker.distribution.manifest.v2+json" \ + -H "Authorization: Bearer token" \ + https://registry.gitlab.com/v2/gitlab-org/gitlab-build-images/blobs/sha256:a3f2e1afa377d20897e08a85cae089393daa0ec019feab3851d592248674b416 +``` + ## 3. Proposal +### 3.1. Shard Container Registry separately to Pods architecture + +Due to it's architecture it extensive architecture and in general highly scalable +horizontal architecture it should be evaluated if the GitLab Container Registry +should be run not in Pod, but in a Cluster and be scaled indepdently. + +This might be easier, but would definitely not offer the same amount of data isolation. + +### 3.2. Run Container Registry within a Pod + +It appears that except `/jwt/auth` which would likely have to be processed by Router +(to decode `scope`) the container registry could be run as a local service of a Pod. + +The actual data at least in case of GitLab.com is not forwarded via registry, +but rather served directly from Object Storage / CDN. + +Its design encodes container repository image in a URL that is easily routable. +It appears that we could re-use the same stateless Router service in front of Container Registry +to serve manifests and blobs redirect. + +The only downside is increased complexity of managing standalone registry for each Pod, +but this might be desired approach. + ## 4. Evaluation +There do not seem any theorethical problems with running GitLab Container Registry in a Pod. +Service seems that can be easily made routable to work well. + +The practical complexities are around managing complex service from infrastructure side. + ## 4.1. Pros ## 4.2. Cons diff --git a/doc/development/testing_guide/best_practices.md b/doc/development/testing_guide/best_practices.md index 80a4604c341..a0bdd9f9eb4 100644 --- a/doc/development/testing_guide/best_practices.md +++ b/doc/development/testing_guide/best_practices.md @@ -859,6 +859,8 @@ it 'is overdue' do travel_to(3.days.from_now) do expect(issue).to be_overdue end + + travel_back # Returns the current time back to its original state end ``` diff --git a/lib/api/api.rb b/lib/api/api.rb index 8bc01cef625..10fdb8d7682 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -182,6 +182,7 @@ module API mount ::API::Branches mount ::API::BroadcastMessages mount ::API::BulkImports + mount ::API::Ci::JobArtifacts mount ::API::Ci::Jobs mount ::API::Ci::ResourceGroups mount ::API::Ci::Runner @@ -282,7 +283,6 @@ module API mount ::API::Admin::Sidekiq mount ::API::AwardEmoji mount ::API::Boards - mount ::API::Ci::JobArtifacts mount ::API::Ci::Pipelines mount ::API::Ci::PipelineSchedules mount ::API::Ci::SecureFiles diff --git a/lib/api/ci/job_artifacts.rb b/lib/api/ci/job_artifacts.rb index 352ad04c982..add844d11c5 100644 --- a/lib/api/ci/job_artifacts.rb +++ b/lib/api/ci/job_artifacts.rb @@ -24,10 +24,19 @@ module API resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do desc 'Download the artifacts archive from a job' do detail 'This feature was introduced in GitLab 8.10' + failure [ + { code: 401, message: 'Unauthorized' }, + { code: 403, message: 'Forbidden' }, + { code: 404, message: 'Not found' } + ] end params do - requires :ref_name, type: String, desc: 'The ref from repository' - requires :job, type: String, desc: 'The name for the job' + requires :ref_name, type: String, + desc: 'Branch or tag name in repository. `HEAD` or `SHA` references are not supported.' + requires :job, type: String, desc: 'The name of the job.' + optional :job_token, type: String, + desc: 'To be used with triggers for multi-project pipelines, ' \ + 'available only on Premium and Ultimate tiers.' end route_setting :authentication, job_token_allowed: true get ':id/jobs/artifacts/:ref_name/download', @@ -43,11 +52,21 @@ module API desc 'Download a specific file from artifacts archive from a ref' do detail 'This feature was introduced in GitLab 11.5' + failure [ + { code: 400, message: 'Bad request' }, + { code: 401, message: 'Unauthorized' }, + { code: 403, message: 'Forbidden' }, + { code: 404, message: 'Not found' } + ] end params do - requires :ref_name, type: String, desc: 'The ref from repository' - requires :job, type: String, desc: 'The name for the job' - requires :artifact_path, type: String, desc: 'Artifact path' + requires :ref_name, type: String, + desc: 'Branch or tag name in repository. `HEAD` or `SHA` references are not supported.' + requires :job, type: String, desc: 'The name of the job.' + requires :artifact_path, type: String, desc: 'Path to a file inside the artifacts archive.' + optional :job_token, type: String, + desc: 'To be used with triggers for multi-project pipelines, ' \ + 'available only on Premium and Ultimate tiers.' end route_setting :authentication, job_token_allowed: true get ':id/jobs/artifacts/:ref_name/raw/*artifact_path', @@ -69,9 +88,17 @@ module API desc 'Download the artifacts archive from a job' do detail 'This feature was introduced in GitLab 8.5' + failure [ + { code: 401, message: 'Unauthorized' }, + { code: 403, message: 'Forbidden' }, + { code: 404, message: 'Not found' } + ] end params do requires :job_id, type: Integer, desc: 'The ID of a job' + optional :job_token, type: String, + desc: 'To be used with triggers for multi-project pipelines, ' \ + 'available only on Premium and Ultimate tiers.' end route_setting :authentication, job_token_allowed: true get ':id/jobs/:job_id/artifacts', urgency: :low do @@ -85,10 +112,19 @@ module API desc 'Download a specific file from artifacts archive' do detail 'This feature was introduced in GitLab 10.0' + failure [ + { code: 400, message: 'Bad request' }, + { code: 401, message: 'Unauthorized' }, + { code: 403, message: 'Forbidden' }, + { code: 404, message: 'Not found' } + ] end params do requires :job_id, type: Integer, desc: 'The ID of a job' - requires :artifact_path, type: String, desc: 'Artifact path' + requires :artifact_path, type: String, desc: 'Path to a file inside the artifacts archive.' + optional :job_token, type: String, + desc: 'To be used with triggers for multi-project pipelines, ' \ + 'available only on Premium and Ultimate tiers.' end route_setting :authentication, job_token_allowed: true get ':id/jobs/:job_id/artifacts/*artifact_path', urgency: :low, format: false do @@ -113,6 +149,11 @@ module API desc 'Keep the artifacts to prevent them from being deleted' do success ::API::Entities::Ci::Job + failure [ + { code: 401, message: 'Unauthorized' }, + { code: 403, message: 'Forbidden' }, + { code: 404, message: 'Not found' } + ] end params do requires :job_id, type: Integer, desc: 'The ID of a job' @@ -132,6 +173,12 @@ module API desc 'Delete the artifacts files from a job' do detail 'This feature was introduced in GitLab 11.9' + success code: 204 + failure [ + { code: 401, message: 'Unauthorized' }, + { code: 403, message: 'Forbidden' }, + { code: 409, message: 'Conflict' } + ] end params do requires :job_id, type: Integer, desc: 'The ID of a job' @@ -148,7 +195,14 @@ module API status :no_content end - desc 'Expire the artifacts files from a project' + desc 'Expire the artifacts files from a project' do + success code: 202 + failure [ + { code: 401, message: 'Unauthorized' }, + { code: 403, message: 'Forbidden' }, + { code: 409, message: 'Conflict' } + ] + end delete ':id/artifacts' do authorize_destroy_artifacts! diff --git a/lib/api/groups.rb b/lib/api/groups.rb index ca99e30fbf7..cb67848247e 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -186,7 +186,7 @@ module API end def check_subscription!(group) - render_api_error!("This group can't be removed because it is linked to a subscription.", :bad_request) if group.paid? + render_api_error!("This group can't be removed because it is linked to a subscription.", :bad_request) if group.prevent_delete? end end diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index de52f3405e3..c826872af01 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -505,6 +505,24 @@ module API access_raw_diffs: to_boolean(params.fetch(:access_raw_diffs, false)) end + desc 'Get the merge request diffs' do + detail 'Get a list of merge request diffs.' + success Entities::Diff + failure [ + { code: 403, message: 'Forbidden' }, + { code: 404, message: 'Not found' } + ] + tags %w[merge_requests] + end + params do + use :pagination + end + get ':id/merge_requests/:merge_request_iid/diffs', feature_category: :code_review, urgency: :low do + merge_request = find_merge_request_with_access(params[:merge_request_iid]) + + present paginate(merge_request.merge_request_diff.paginated_diffs(params[:page], params[:per_page])).diffs, with: Entities::Diff + end + desc 'Get single merge request pipelines' do detail 'Get a list of merge request pipelines.' success Entities::Ci::PipelineBasic diff --git a/lib/gitlab/diff/file_collection/compare.rb b/lib/gitlab/diff/file_collection/compare.rb index 6d8395d048d..df0d71f3db2 100644 --- a/lib/gitlab/diff/file_collection/compare.rb +++ b/lib/gitlab/diff/file_collection/compare.rb @@ -4,7 +4,15 @@ module Gitlab module Diff module FileCollection class Compare < Base + delegate :limit_value, :current_page, :next_page, :prev_page, :total_count, :total_pages, to: :@pagination + def initialize(compare, project:, diff_options:, diff_refs: nil) + @pagination = Gitlab::PaginationDelegate.new( + page: diff_options&.delete(:page), + per_page: diff_options&.delete(:per_page), + count: diff_options&.delete(:count) + ) + super(compare, project: project, diff_options: diff_options, diff --git a/lib/gitlab/diff/file_collection/merge_request_diff_batch.rb b/lib/gitlab/diff/file_collection/merge_request_diff_batch.rb index 0a601bde612..56027d6a4de 100644 --- a/lib/gitlab/diff/file_collection/merge_request_diff_batch.rb +++ b/lib/gitlab/diff/file_collection/merge_request_diff_batch.rb @@ -10,6 +10,8 @@ module Gitlab # separate file keys (https://gitlab.com/gitlab-org/gitlab/issues/30550). # class MergeRequestDiffBatch < MergeRequestDiffBase + include PaginatedDiffs + DEFAULT_BATCH_PAGE = 1 DEFAULT_BATCH_SIZE = 30 @@ -25,41 +27,8 @@ module Gitlab } end - override :diffs - def diffs - strong_memoize(:diffs) do - @merge_request_diff.opening_external_diff do - # Avoiding any extra queries. - collection = @paginated_collection.to_a - - # The offset collection and calculation is required so that we - # know how much has been loaded in previous batches, collapsing - # the current paginated set accordingly (collection limit calculation). - # See: https://docs.gitlab.com/ee/development/diffs.html#diff-collection-limits - # - offset_index = collection.first&.index - options = diff_options.dup - - collection = - if offset_index && offset_index > 0 - offset_collection = relation.limit(offset_index) # rubocop:disable CodeReuse/ActiveRecord - options[:offset_index] = offset_index - offset_collection + collection - else - collection - end - - Gitlab::Git::DiffCollection.new(collection.map(&:to_hash), options) - end - end - end - private - def relation - @merge_request_diff.merge_request_diff_files - end - # rubocop: disable CodeReuse/ActiveRecord def load_paginated_collection(batch_page, batch_size, diff_options) batch_page ||= DEFAULT_BATCH_PAGE diff --git a/lib/gitlab/diff/file_collection/paginated_diffs.rb b/lib/gitlab/diff/file_collection/paginated_diffs.rb new file mode 100644 index 00000000000..63c186affe9 --- /dev/null +++ b/lib/gitlab/diff/file_collection/paginated_diffs.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +module Gitlab + module Diff + module FileCollection + module PaginatedDiffs + include Gitlab::Utils::StrongMemoize + extend ::Gitlab::Utils::Override + + override :diffs + def diffs + merge_request_diff.opening_external_diff do + # Avoiding any extra queries. + collection = paginated_collection.to_a + + # The offset collection and calculation is required so that we + # know how much has been loaded in previous batches, collapsing + # the current paginated set accordingly (collection limit calculation). + # See: https://docs.gitlab.com/ee/development/diffs.html#diff-collection-limits + # + offset_index = collection.first&.index + options = diff_options.dup + + collection = + if offset_index && offset_index > 0 + offset_collection = relation.limit(offset_index) # rubocop:disable CodeReuse/ActiveRecord + options[:offset_index] = offset_index + offset_collection + collection + else + collection + end + + Gitlab::Git::DiffCollection.new(collection.map(&:to_hash), options) + end + end + strong_memoize_attr :diffs + + private + + attr_reader :merge_request_diff, :paginated_collection + + def relation + merge_request_diff.merge_request_diff_files + end + end + end + end +end diff --git a/lib/gitlab/diff/file_collection/paginated_merge_request_diff.rb b/lib/gitlab/diff/file_collection/paginated_merge_request_diff.rb new file mode 100644 index 00000000000..37abad81305 --- /dev/null +++ b/lib/gitlab/diff/file_collection/paginated_merge_request_diff.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +module Gitlab + module Diff + module FileCollection + # Builds a traditional paginated diff file collection using Kaminari + # `per` and `per_page` which is different from how `MergeRequestDiffBatch` + # works (e.g. supports gradual loading). + class PaginatedMergeRequestDiff < MergeRequestDiffBase + include PaginatedDiffs + + DEFAULT_PAGE = 1 + DEFAULT_PER_PAGE = 30 + + delegate :limit_value, :current_page, :next_page, :prev_page, :total_count, + :total_pages, to: :paginated_collection + + def initialize(merge_request_diff, page, per_page) + super(merge_request_diff, diff_options: nil) + + @paginated_collection = load_paginated_collection(page, per_page) + end + + private + + def load_paginated_collection(page, per_page) + page ||= DEFAULT_PAGE + per_page ||= DEFAULT_PER_PAGE + + relation.page(page).per([per_page.to_i, DEFAULT_PER_PAGE].min) + end + end + end + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 377f499dd1d..abea652b88b 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -5899,9 +5899,6 @@ msgstr "" msgid "Available on-demand" msgstr "" -msgid "Available shared runners:" -msgstr "" - msgid "Available specific runners" msgstr "" @@ -8232,7 +8229,7 @@ msgstr "" msgid "Checkout|Failed to load states. Please try again." msgstr "" -msgid "Checkout|Failed to load the payment form. Please try again." +msgid "Checkout|Failed to load the payment form. Refresh the page and try again." msgstr "" msgid "Checkout|Failed to register credit card. Please try again." @@ -35230,6 +35227,9 @@ msgstr "" msgid "Runners|Available" msgstr "" +msgid "Runners|Available shared runners: %{count}" +msgstr "" + msgid "Runners|Available to all projects" msgstr "" diff --git a/spec/controllers/projects/settings/ci_cd_controller_spec.rb b/spec/controllers/projects/settings/ci_cd_controller_spec.rb index e847225b85b..214031a0fb2 100644 --- a/spec/controllers/projects/settings/ci_cd_controller_spec.rb +++ b/spec/controllers/projects/settings/ci_cd_controller_spec.rb @@ -19,8 +19,10 @@ RSpec.describe Projects::Settings::CiCdController do let_it_be(:group) { create(:group, parent: parent_group) } let_it_be(:other_project) { create(:project, group: group) } + subject { get :show, params: { namespace_id: project.namespace, project_id: project } } + it 'renders show with 200 status code' do - get :show, params: { namespace_id: project.namespace, project_id: project } + subject expect(response).to have_gitlab_http_status(:ok) expect(response).to render_template(:show) @@ -32,27 +34,40 @@ RSpec.describe Projects::Settings::CiCdController do end it 'renders show with 404 status code' do - get :show, params: { namespace_id: project.namespace, project_id: project } + subject + expect(response).to have_gitlab_http_status(:not_found) end end - context 'with group runners' do - let_it_be(:group_runner) { create(:ci_runner, :group, groups: [group]) } + context 'with assignable project runners' do let_it_be(:project_runner) { create(:ci_runner, :project, projects: [other_project]) } - let_it_be(:shared_runner) { create(:ci_runner, :instance) } before do group.add_maintainer(user) end - subject { get :show, params: { namespace_id: project.namespace, project_id: project } } - - it 'sets assignable project runners only' do + it 'sets assignable project runners' do subject expect(assigns(:assignable_runners)).to contain_exactly(project_runner) end + end + + context 'with group runners' do + let_it_be(:group) { create :group } + let_it_be(:project) { create :project, group: group } + let_it_be(:group_runner) { create(:ci_runner, :group, groups: [group]) } + + it 'sets group runners' do + subject + + expect(assigns(:group_runners)).to contain_exactly(group_runner) + end + end + + context 'with instance runners' do + let_it_be(:shared_runner) { create(:ci_runner, :instance) } it 'sets shared runners' do subject diff --git a/spec/features/runners_spec.rb b/spec/features/runners_spec.rb index b732fafe571..b971663781c 100644 --- a/spec/features/runners_spec.rb +++ b/spec/features/runners_spec.rb @@ -123,7 +123,15 @@ RSpec.describe 'Runners' do end context 'when multiple shared runners are configured' do - let!(:shared_runner_2) { create(:ci_runner, :instance) } + let_it_be(:shared_runner_2) { create(:ci_runner, :instance) } + + it 'shows the runner count' do + visit project_runners_path(project) + + within '[data-testid="available-shared-runners"]' do + expect(page).to have_content format(_('Available shared runners: %{count}'), { count: 2 }) + end + end it 'adds pagination to the shared runner list' do stub_const('Projects::Settings::CiCdController::NUMBER_OF_RUNNERS_PER_PAGE', 1) @@ -322,7 +330,7 @@ RSpec.describe 'Runners' do end context 'project with a group and a group runner' do - let_it_be(:ci_runner) do + let_it_be(:group_runner) do create(:ci_runner, :group, groups: [group], description: 'group-runner') end @@ -346,6 +354,28 @@ RSpec.describe 'Runners' do expect(page).to have_content 'Disable group runners' expect(project.reload.group_runners_enabled).to be true end + + context 'when multiple group runners are configured' do + let_it_be(:group_runner_2) { create(:ci_runner, :group, groups: [group]) } + + it 'shows the runner count' do + visit project_runners_path(project) + + within '[data-testid="group-runners"]' do + expect(page).to have_content format(_('Available group runners: %{runners}'), { runners: 2 }) + end + end + + it 'adds pagination to the group runner list' do + stub_const('Projects::Settings::CiCdController::NUMBER_OF_RUNNERS_PER_PAGE', 1) + + visit project_runners_path(project) + + within '[data-testid="group-runners"]' do + expect(find('.pagination')).not_to be_nil + end + end + end end end end diff --git a/spec/helpers/groups/settings_helper_spec.rb b/spec/helpers/groups/settings_helper_spec.rb index f8c0bfc19a1..ed948f5456c 100644 --- a/spec/helpers/groups/settings_helper_spec.rb +++ b/spec/helpers/groups/settings_helper_spec.rb @@ -11,8 +11,7 @@ RSpec.describe Groups::SettingsHelper do using RSpec::Parameterized::TableSyntax fake_form_id = "fake_form_id" - - where(:is_paid, :is_button_disabled, :form_value_id) do + where(:prevent_delete_response, :is_button_disabled, :form_value_id) do true | "true" | nil true | "true" | fake_form_id false | "false" | nil @@ -21,7 +20,7 @@ RSpec.describe Groups::SettingsHelper do with_them do it "returns expected parameters" do - allow(group).to receive(:paid?).and_return(is_paid) + allow(group).to receive(:prevent_delete?).and_return(prevent_delete_response) expected = helper.group_settings_confirm_modal_data(group, form_value_id) expect(expected).to eq({ diff --git a/spec/lib/gitlab/diff/file_collection/compare_spec.rb b/spec/lib/gitlab/diff/file_collection/compare_spec.rb index ce70903a480..c3f768db7f0 100644 --- a/spec/lib/gitlab/diff/file_collection/compare_spec.rb +++ b/spec/lib/gitlab/diff/file_collection/compare_spec.rb @@ -16,10 +16,11 @@ RSpec.describe Gitlab::Diff::FileCollection::Compare do end let(:diffable) { Compare.new(raw_compare, project) } + let(:diff_options) { {} } let(:collection_default_args) do { project: diffable.project, - diff_options: {}, + diff_options: diff_options, diff_refs: diffable.diff_refs } end @@ -65,4 +66,32 @@ RSpec.describe Gitlab::Diff::FileCollection::Compare do expect(cache_key).to eq ['compare', head_commit.id, start_commit.id] end end + + describe 'pagination methods' do + subject(:compare) { described_class.new(diffable, **collection_default_args) } + + context 'when pagination options are not present' do + it 'returns default values' do + expect(compare.limit_value).to eq(Kaminari.config.default_per_page) + expect(compare.current_page).to eq(1) + expect(compare.next_page).to be_nil + expect(compare.prev_page).to be_nil + expect(compare.total_count).to be_nil + expect(compare.total_pages).to eq(0) + end + end + + context 'when pagination options are present' do + let(:diff_options) { { page: 1, per_page: 10, count: 20 } } + + it 'returns values based on options' do + expect(compare.limit_value).to eq(10) + expect(compare.current_page).to eq(1) + expect(compare.next_page).to eq(2) + expect(compare.prev_page).to be_nil + expect(compare.total_count).to eq(20) + expect(compare.total_pages).to eq(2) + end + end + end end diff --git a/spec/lib/gitlab/diff/file_collection/merge_request_diff_batch_spec.rb b/spec/lib/gitlab/diff/file_collection/merge_request_diff_batch_spec.rb index beb85d383a0..9ac242459bf 100644 --- a/spec/lib/gitlab/diff/file_collection/merge_request_diff_batch_spec.rb +++ b/spec/lib/gitlab/diff/file_collection/merge_request_diff_batch_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Diff::FileCollection::MergeRequestDiffBatch do +RSpec.describe Gitlab::Diff::FileCollection::MergeRequestDiffBatch, feature_category: :code_review do let(:merge_request) { create(:merge_request) } let(:batch_page) { 0 } let(:batch_size) { 10 } diff --git a/spec/lib/gitlab/diff/file_collection/paginated_merge_request_diff_spec.rb b/spec/lib/gitlab/diff/file_collection/paginated_merge_request_diff_spec.rb new file mode 100644 index 00000000000..74e5e667702 --- /dev/null +++ b/spec/lib/gitlab/diff/file_collection/paginated_merge_request_diff_spec.rb @@ -0,0 +1,114 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Diff::FileCollection::PaginatedMergeRequestDiff, feature_category: :code_review do + let(:merge_request) { create(:merge_request) } + let(:page) { 1 } + let(:per_page) { 10 } + let(:diffable) { merge_request.merge_request_diff } + let(:diff_files_relation) { diffable.merge_request_diff_files } + let(:diff_files) { subject.diff_files } + + subject do + described_class.new(diffable, + page, + per_page) + end + + describe '#diff_files' do + let(:per_page) { 3 } + let(:paginated_rel) { diff_files_relation.page(page).per(per_page) } + + let(:expected_batch_files) do + paginated_rel.map(&:new_path) + end + + it 'returns paginated diff files' do + expect(diff_files.size).to eq(3) + end + + it 'returns a valid instance of a DiffCollection' do + expect(diff_files).to be_a(Gitlab::Git::DiffCollection) + end + + context 'when first page' do + it 'returns correct diff files' do + expect(diff_files.map(&:new_path)).to eq(expected_batch_files) + end + end + + context 'when another page' do + let(:page) { 2 } + + it 'returns correct diff files' do + expect(diff_files.map(&:new_path)).to eq(expected_batch_files) + end + end + + context 'when page is nil' do + let(:page) { nil } + + it 'returns correct diff files' do + expected_batch_files = + diff_files_relation.page(described_class::DEFAULT_PAGE).per(per_page).map(&:new_path) + + expect(diff_files.map(&:new_path)).to eq(expected_batch_files) + end + end + + context 'when per_page is nil' do + let(:per_page) { nil } + + it 'returns correct diff files' do + expected_batch_files = + diff_files_relation.page(page).per(described_class::DEFAULT_PER_PAGE).map(&:new_path) + + expect(diff_files.map(&:new_path)).to eq(expected_batch_files) + end + end + + context 'when invalid page' do + let(:page) { 999 } + + it 'returns correct diff files' do + expect(diff_files.map(&:new_path)).to be_empty + end + end + + context 'when last page' do + it 'returns correct diff files' do + last_page = diff_files_relation.count - per_page + collection = described_class.new(diffable, + last_page, + per_page) + + expected_batch_files = diff_files_relation.page(last_page).per(per_page).map(&:new_path) + + expect(collection.diff_files.map(&:new_path)).to eq(expected_batch_files) + end + end + end + + it_behaves_like 'unfoldable diff' do + subject do + described_class.new(merge_request.merge_request_diff, + page, + per_page) + end + end + + it_behaves_like 'cacheable diff collection' do + let(:cacheable_files_count) { per_page } + end + + it_behaves_like 'unsortable diff files' do + let(:diffable) { merge_request.merge_request_diff } + + subject do + described_class.new(merge_request.merge_request_diff, + page, + per_page) + end + end +end diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb index 22fed716897..a17b90930f0 100644 --- a/spec/models/merge_request_diff_spec.rb +++ b/spec/models/merge_request_diff_spec.rb @@ -507,6 +507,50 @@ RSpec.describe MergeRequestDiff do end end + describe '#paginated_diffs' do + context 'when no persisted files available' do + before do + diff_with_commits.clean! + end + + it 'returns a Gitlab::Diff::FileCollection::Compare' do + diffs = diff_with_commits.paginated_diffs(1, 10) + + expect(diffs).to be_a(Gitlab::Diff::FileCollection::Compare) + expect(diffs.diff_files.size).to eq(10) + end + end + + context 'when persisted files available' do + it 'returns paginated diffs' do + diffs = diff_with_commits.paginated_diffs(1, 10) + + expect(diffs).to be_a(Gitlab::Diff::FileCollection::PaginatedMergeRequestDiff) + expect(diffs.diff_files.size).to eq(10) + end + + it 'sorts diff files directory first' do + diff_with_commits.update!(sorted: false) # Mark as unsorted so it'll re-order + + # There will be 11 returned, as we have to take into account for new and old paths + expect(diff_with_commits.paginated_diffs(1, 10).diff_paths).to eq( + [ + 'bar/branch-test.txt', + 'custom-highlighting/test.gitlab-custom', + 'encoding/iso8859.txt', + 'files/images/wm.svg', + 'files/js/commit.js.coffee', + 'files/js/commit.coffee', + 'files/lfs/lfs_object.iso', + 'files/ruby/popen.rb', + 'files/ruby/regex.rb', + 'files/.DS_Store', + 'files/whitespace' + ]) + end + end + end + describe '#diffs' do let(:diff_options) { {} } diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index eea223485ce..18d3fdb2d50 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -1788,6 +1788,58 @@ RSpec.describe API::MergeRequests do end end + describe 'GET /projects/:id/merge_requests/:merge_request_iid/diffs' do + let_it_be(:merge_request) do + create( + :merge_request, + :simple, + author: user, + assignees: [user], + source_project: project, + target_project: project, + source_branch: 'markdown', + title: "Test", + created_at: base_time + ) + end + + it 'returns a 404 when merge_request_iid not found' do + get api("/projects/#{project.id}/merge_requests/0/diffs", user) + expect(response).to have_gitlab_http_status(:not_found) + end + + it 'returns a 404 when merge_request id is used instead of iid' do + get api("/projects/#{project.id}/merge_requests/#{merge_request.id}/diffs", user) + + expect(response).to have_gitlab_http_status(:not_found) + end + + context 'when merge request author has only guest access' do + it_behaves_like 'rejects user from accessing merge request info' do + let(:url) { "/projects/#{project.id}/merge_requests/#{merge_request.iid}/diffs" } + end + end + + it 'returns the diffs of the merge_request' do + get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/diffs", user) + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.size).to eq(merge_request.diffs.size) + end + + context 'when pagination params are present' do + it 'returns limited diffs' do + get( + api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/diffs", user), + params: { page: 1, per_page: 1 } + ) + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.size).to eq(1) + end + end + end + describe 'GET /projects/:id/merge_requests/:merge_request_iid/pipelines' do let_it_be(:merge_request) { create(:merge_request, :simple, author: user, assignees: [user], source_project: project, target_project: project, source_branch: 'markdown', title: "Test", created_at: base_time) } diff --git a/spec/tooling/danger/specs_spec.rb b/spec/tooling/danger/specs_spec.rb index a0d0b03a0da..dcc1f592062 100644 --- a/spec/tooling/danger/specs_spec.rb +++ b/spec/tooling/danger/specs_spec.rb @@ -225,7 +225,7 @@ RSpec.describe Tooling::Danger::Specs, feature_category: :tooling do %<suggested_line>s ``` - Consider addding `feature_category: <feature_category_name>` for this example if it is not set already. + Consider adding `feature_category: <feature_category_name>` for this example if it is not set already. See [testing best practices](https://docs.gitlab.com/ee/development/testing_guide/best_practices.html#feature-category-metadata). SUGGESTION_MARKDOWN end diff --git a/tooling/danger/specs.rb b/tooling/danger/specs.rb index c2427f56f33..c7baf920314 100644 --- a/tooling/danger/specs.rb +++ b/tooling/danger/specs.rb @@ -47,7 +47,7 @@ module Tooling FEATURE_CATEGORY_REGEX = /^\+.?RSpec\.describe(.+)(?!feature_category)/.freeze FEATURE_CATEGORY_SUGGESTION = <<~SUGGESTION_MARKDOWN - Consider addding `feature_category: <feature_category_name>` for this example if it is not set already. + Consider adding `feature_category: <feature_category_name>` for this example if it is not set already. See [testing best practices](https://docs.gitlab.com/ee/development/testing_guide/best_practices.html#feature-category-metadata). SUGGESTION_MARKDOWN FEATURE_CATEGORY_EXCLUDE = 'feature_category' |