diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue
index 2feca0bd7a5..b80262622eb 100644
--- a/app/assets/javascripts/notes/components/noteable_discussion.vue
+++ b/app/assets/javascripts/notes/components/noteable_discussion.vue
@@ -170,6 +170,7 @@ export default {
return {
'is-replying gl-pt-0!': this.isReplying,
'internal-note': this.isDiscussionInternal,
+ 'gl-pt-0!': !this.discussion.diff_discussion && this.isReplying,
};
},
},
diff --git a/app/assets/javascripts/notes/mixins/diff_line_note_form.js b/app/assets/javascripts/notes/mixins/diff_line_note_form.js
index 20ffcda282a..1ea78f18aca 100644
--- a/app/assets/javascripts/notes/mixins/diff_line_note_form.js
+++ b/app/assets/javascripts/notes/mixins/diff_line_note_form.js
@@ -1,6 +1,10 @@
import { mapActions, mapGetters, mapState } from 'vuex';
import { getDraftReplyFormData, getDraftFormData } from '~/batch_comments/utils';
-import { TEXT_DIFF_POSITION_TYPE, IMAGE_DIFF_POSITION_TYPE } from '~/diffs/constants';
+import {
+ TEXT_DIFF_POSITION_TYPE,
+ IMAGE_DIFF_POSITION_TYPE,
+ FILE_DIFF_POSITION_TYPE,
+} from '~/diffs/constants';
import { createAlert } from '~/alert';
import { clearDraft } from '~/lib/utils/autosave';
import { s__ } from '~/locale';
@@ -18,7 +22,7 @@ export default {
...mapState('diffs', ['commit', 'showWhitespace']),
},
methods: {
- ...mapActions('diffs', ['cancelCommentForm']),
+ ...mapActions('diffs', ['cancelCommentForm', 'toggleFileCommentForm']),
...mapActions('batchComments', ['addDraftToReview', 'saveDraft', 'insertDraftIntoDrafts']),
addReplyToReview(noteText, isResolving) {
const postData = getDraftReplyFormData({
@@ -47,13 +51,13 @@ export default {
});
});
},
- addToReview(note) {
+ addToReview(note, positionType = null) {
const lineRange =
(this.line && this.commentLineStart && formatLineRange(this.commentLineStart, this.line)) ||
{};
- const positionType = this.diffFileCommentForm
- ? IMAGE_DIFF_POSITION_TYPE
- : TEXT_DIFF_POSITION_TYPE;
+ const position =
+ positionType ||
+ (this.diffFileCommentForm ? IMAGE_DIFF_POSITION_TYPE : TEXT_DIFF_POSITION_TYPE);
const selectedDiffFile = this.getDiffFileByHash(this.diffFileHash);
const postData = getDraftFormData({
note,
@@ -64,7 +68,7 @@ export default {
diffViewType: this.diffViewType,
diffFile: selectedDiffFile,
linePosition: this.position,
- positionType,
+ positionType: position,
...this.diffFileCommentForm,
lineRange,
showWhitespace: this.showWhitespace,
@@ -76,10 +80,12 @@ export default {
return this.saveDraft(postData)
.then(() => {
- if (positionType === IMAGE_DIFF_POSITION_TYPE) {
+ if (position === IMAGE_DIFF_POSITION_TYPE) {
this.closeDiffFileCommentForm(this.diffFileHash);
- } else {
+ } else if (this.line?.line_code) {
this.handleClearForm(this.line.line_code);
+ } else if (position === FILE_DIFF_POSITION_TYPE) {
+ this.toggleFileCommentForm(this.diffFile.file_path);
}
})
.catch(() => {
diff --git a/app/assets/stylesheets/page_bundles/merge_requests.scss b/app/assets/stylesheets/page_bundles/merge_requests.scss
index c59bdd70ba8..fc4a9d3dff9 100644
--- a/app/assets/stylesheets/page_bundles/merge_requests.scss
+++ b/app/assets/stylesheets/page_bundles/merge_requests.scss
@@ -1271,3 +1271,32 @@ $tabs-holder-z-index: 250;
margin-right: 8px;
border: 2px solid var(--gray-50, $gray-50);
}
+
+.diff-file-discussions-wrapper {
+ @include gl-w-full;
+
+ max-width: 800px;
+
+ .diff-discussions > .notes {
+ @include gl-p-5;
+ }
+
+ .diff-discussions:not(:first-child) >.notes {
+ @include gl-pt-0;
+ }
+
+ .note-discussion {
+ @include gl-rounded-base;
+
+ border: 1px solid var(--gray-100, $gray-100) !important;
+ }
+
+ .discussion-collapsible {
+ @include gl-m-0;
+ @include gl-border-l-0;
+ @include gl-border-r-0;
+ @include gl-border-b-0;
+ @include gl-rounded-top-left-none;
+ @include gl-rounded-top-right-none;
+ }
+}
diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss
index b31ee069236..069ccad4435 100644
--- a/app/assets/stylesheets/pages/notes.scss
+++ b/app/assets/stylesheets/pages/notes.scss
@@ -667,7 +667,7 @@ $system-note-icon-m-left: $avatar-m-left + $icon-size-diff / $avatar-m-ratio;
.discussion-reply-holder {
border-top: 0;
- border-radius: 0 0 $gl-border-radius-base $gl-border-radius-base;
+ border-radius: $gl-border-radius-base $gl-border-radius-base;
position: relative;
.discussion-form {
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 2f37382da73..2b450239914 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -53,6 +53,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
push_force_frontend_feature_flag(:summarize_my_code_review, summarize_my_code_review_enabled?)
push_frontend_feature_flag(:mr_activity_filters, current_user)
push_frontend_feature_flag(:review_apps_redeploy_mr_widget, project)
+ push_frontend_feature_flag(:comment_on_files, current_user)
end
around_action :allow_gitaly_ref_name_caching, only: [:index, :show, :diffs, :discussions]
diff --git a/app/helpers/ci/secure_files_helper.rb b/app/helpers/ci/secure_files_helper.rb
index fca89ddab1e..c4cc178f930 100644
--- a/app/helpers/ci/secure_files_helper.rb
+++ b/app/helpers/ci/secure_files_helper.rb
@@ -3,6 +3,7 @@ module Ci
module SecureFilesHelper
def show_secure_files_setting(project, user)
return false if user.nil?
+ return false unless Gitlab.config.ci_secure_files.enabled
user.can?(:read_secure_files, project)
end
diff --git a/app/models/concerns/diff_positionable_note.rb b/app/models/concerns/diff_positionable_note.rb
index 7a6076c7d2e..b10b318fb7c 100644
--- a/app/models/concerns/diff_positionable_note.rb
+++ b/app/models/concerns/diff_positionable_note.rb
@@ -4,7 +4,7 @@ module DiffPositionableNote
included do
before_validation :set_original_position, on: :create
- before_validation :update_position, on: :create, if: :on_text?, unless: :importing?
+ before_validation :update_position, on: :create, if: :should_update_position?, unless: :importing?
serialize :original_position, Gitlab::Diff::Position # rubocop:disable Cop/ActiveRecordSerialize
serialize :position, Gitlab::Diff::Position # rubocop:disable Cop/ActiveRecordSerialize
@@ -37,10 +37,18 @@ module DiffPositionableNote
end
end
+ def should_update_position?
+ on_text? || on_file?
+ end
+
def on_text?
!!position&.on_text?
end
+ def on_file?
+ !!position&.on_file?
+ end
+
def on_image?
!!position&.on_image?
end
diff --git a/app/models/note.rb b/app/models/note.rb
index 4b65103aa9f..cb6d47d0d6c 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -689,6 +689,7 @@ class Note < ApplicationRecord
def show_outdated_changes?
return false unless for_merge_request?
return false unless system?
+ return false if change_position&.on_file?
return false unless change_position&.line_range
change_position.line_range["end"] || change_position.line_range["start"]
diff --git a/config/feature_flags/development/comment_on_files.yml b/config/feature_flags/development/comment_on_files.yml
new file mode 100644
index 00000000000..fc79ee249a4
--- /dev/null
+++ b/config/feature_flags/development/comment_on_files.yml
@@ -0,0 +1,8 @@
+---
+name: comment_on_files
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/121429
+rollout_issue_url:
+milestone: '16.1'
+type: development
+group: group::code review
+default_enabled: false
diff --git a/config/object_store_settings.rb b/config/object_store_settings.rb
index 9cc037d04a8..22194d8c2de 100644
--- a/config/object_store_settings.rb
+++ b/config/object_store_settings.rb
@@ -2,7 +2,7 @@
# Set default values for object_store settings
class ObjectStoreSettings
- SUPPORTED_TYPES = %w(artifacts external_diffs lfs uploads packages dependency_proxy terraform_state pages ci_secure_files).freeze
+ SUPPORTED_TYPES = %w(artifacts external_diffs lfs uploads packages dependency_proxy terraform_state pages).freeze
ALLOWED_OBJECT_STORE_OVERRIDES = %w(bucket enabled proxy_download cdn).freeze
# To ensure the one Workhorse credential matches the Rails config, we
diff --git a/db/fixtures/development/36_achievements.rb b/db/fixtures/development/36_achievements.rb
index 5798fd2d6c7..7a435daf82e 100644
--- a/db/fixtures/development/36_achievements.rb
+++ b/db/fixtures/development/36_achievements.rb
@@ -58,7 +58,7 @@ class Gitlab::Seeder::Achievements
end
Gitlab::Seeder.quiet do
- puts "\nGenerating ahievements"
+ puts "\nGenerating achievements"
group = Group.first
users = User.first(4).pluck(:id)
diff --git a/db/migrate/20230529173607_add_id_column_to_pm_checkpoints.rb b/db/migrate/20230529173607_add_id_column_to_pm_checkpoints.rb
new file mode 100644
index 00000000000..55135211ce8
--- /dev/null
+++ b/db/migrate/20230529173607_add_id_column_to_pm_checkpoints.rb
@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+class AddIdColumnToPmCheckpoints < Gitlab::Database::Migration[2.1]
+ enable_lock_retries!
+
+ def up
+ add_column(:pm_checkpoints, :id, :bigserial)
+ add_index(:pm_checkpoints, :id, unique: true, name: :pm_checkpoints_unique_index) # rubocop:disable Migration/AddIndex
+ add_index(:pm_checkpoints, [:purl_type, :data_type, :version_format], unique: true, # rubocop:disable Migration/AddIndex
+ name: :pm_checkpoints_path_components)
+ swap_primary_key(:pm_checkpoints, :pm_checkpoints_pkey, :pm_checkpoints_unique_index)
+ end
+
+ def down
+ add_index(:pm_checkpoints, [:purl_type, :data_type, :version_format], unique: true, # rubocop:disable Migration/AddIndex
+ name: :pm_checkpoints_unique_index)
+ remove_index(:pm_checkpoints, name: :pm_checkpoints_path_components) # rubocop:disable Migration/RemoveIndex
+ unswap_primary_key(:pm_checkpoints, :pm_checkpoints_pkey, :pm_checkpoints_unique_index)
+ remove_column(:pm_checkpoints, :id)
+ end
+end
diff --git a/db/schema_migrations/20230529173607 b/db/schema_migrations/20230529173607
new file mode 100644
index 00000000000..f0f22512414
--- /dev/null
+++ b/db/schema_migrations/20230529173607
@@ -0,0 +1 @@
+5b2b9a7c58ed2153a70e4f32dbd8a4c4c9976afce478d761ced2ae9de43ea377
\ No newline at end of file
diff --git a/db/structure.sql b/db/structure.sql
index 23e2df53c91..d510a6ab9fb 100644
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -20311,9 +20311,19 @@ CREATE TABLE pm_checkpoints (
purl_type smallint NOT NULL,
chunk smallint NOT NULL,
data_type smallint DEFAULT 1 NOT NULL,
- version_format smallint DEFAULT 1 NOT NULL
+ version_format smallint DEFAULT 1 NOT NULL,
+ id bigint NOT NULL
);
+CREATE SEQUENCE pm_checkpoints_id_seq
+ START WITH 1
+ INCREMENT BY 1
+ NO MINVALUE
+ NO MAXVALUE
+ CACHE 1;
+
+ALTER SEQUENCE pm_checkpoints_id_seq OWNED BY pm_checkpoints.id;
+
CREATE TABLE pm_licenses (
id bigint NOT NULL,
spdx_identifier text NOT NULL,
@@ -25668,6 +25678,8 @@ ALTER TABLE ONLY pm_advisories ALTER COLUMN id SET DEFAULT nextval('pm_advisorie
ALTER TABLE ONLY pm_affected_packages ALTER COLUMN id SET DEFAULT nextval('pm_affected_packages_id_seq'::regclass);
+ALTER TABLE ONLY pm_checkpoints ALTER COLUMN id SET DEFAULT nextval('pm_checkpoints_id_seq'::regclass);
+
ALTER TABLE ONLY pm_licenses ALTER COLUMN id SET DEFAULT nextval('pm_licenses_id_seq'::regclass);
ALTER TABLE ONLY pm_package_version_licenses ALTER COLUMN id SET DEFAULT nextval('pm_package_version_licenses_id_seq'::regclass);
@@ -27977,7 +27989,7 @@ ALTER TABLE ONLY pm_affected_packages
ADD CONSTRAINT pm_affected_packages_pkey PRIMARY KEY (id);
ALTER TABLE ONLY pm_checkpoints
- ADD CONSTRAINT pm_checkpoints_pkey PRIMARY KEY (purl_type, data_type, version_format);
+ ADD CONSTRAINT pm_checkpoints_pkey PRIMARY KEY (id);
ALTER TABLE ONLY pm_licenses
ADD CONSTRAINT pm_licenses_pkey PRIMARY KEY (id);
@@ -33506,6 +33518,8 @@ CREATE UNIQUE INDEX partial_index_sop_configs_on_project_id ON security_orchestr
CREATE INDEX partial_index_user_id_app_id_created_at_token_not_revoked ON oauth_access_tokens USING btree (resource_owner_id, application_id, created_at) WHERE (revoked_at IS NULL);
+CREATE UNIQUE INDEX pm_checkpoints_path_components ON pm_checkpoints USING btree (purl_type, data_type, version_format);
+
CREATE INDEX scan_finding_approval_mr_rule_index_id ON approval_merge_request_rules USING btree (id) WHERE (report_type = 4);
CREATE INDEX scan_finding_approval_mr_rule_index_merge_request_id ON approval_merge_request_rules USING btree (merge_request_id) WHERE (report_type = 4);
diff --git a/doc/ci/environments/configure_kubernetes_deployments.md b/doc/ci/environments/configure_kubernetes_deployments.md
new file mode 100644
index 00000000000..e618aae7cae
--- /dev/null
+++ b/doc/ci/environments/configure_kubernetes_deployments.md
@@ -0,0 +1,58 @@
+---
+stage: Deploy
+group: Environments
+info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments
+type: reference
+---
+
+# Configure Kubernetes deployments (deprecated)
+
+> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/27630) in GitLab 12.6.
+> - [Deprecated](https://gitlab.com/groups/gitlab-org/configure/-/epics/8) in GitLab 14.5.
+
+WARNING:
+This feature was [deprecated](https://gitlab.com/groups/gitlab-org/configure/-/epics/8) in GitLab 14.5.
+
+If you are deploying to a [Kubernetes cluster](../../user/infrastructure/clusters/index.md)
+associated with your project, you can configure these deployments from your
+`.gitlab-ci.yml` file.
+
+NOTE:
+Kubernetes configuration isn't supported for Kubernetes clusters
+[managed by GitLab](../../user/project/clusters/gitlab_managed_clusters.md).
+
+The following configuration options are supported:
+
+- [`namespace`](https://kubernetes.io/docs/concepts/overview/working-with-objects/namespaces/)
+
+In the following example, the job deploys your application to the
+`production` Kubernetes namespace.
+
+```yaml
+deploy:
+ stage: deploy
+ script:
+ - echo "Deploy to production server"
+ environment:
+ name: production
+ url: https://example.com
+ kubernetes:
+ namespace: production
+ rules:
+ - if: $CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH
+```
+
+When you use the GitLab Kubernetes integration to deploy to a Kubernetes cluster,
+you can view cluster and namespace information. On the deployment
+job page, it's displayed above the job trace:
+
+
+
+## Configure incremental rollouts
+
+Learn how to release production changes to only a portion of your Kubernetes pods with
+[incremental rollouts](../environments/incremental_rollouts.md).
+
+## Related topics
+
+- [Deploy boards (deprecated)](../../user/project/deploy_boards.md)
diff --git a/doc/ci/environments/index.md b/doc/ci/environments/index.md
index 137bc883072..d99c9aac34d 100644
--- a/doc/ci/environments/index.md
+++ b/doc/ci/environments/index.md
@@ -300,54 +300,6 @@ The `when: manual` action:
You can find the play button in the pipelines, environments, deployments, and jobs views.
-## Configure Kubernetes deployments (deprecated)
-
-> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/27630) in GitLab 12.6.
-> - [Deprecated](https://gitlab.com/groups/gitlab-org/configure/-/epics/8) in GitLab 14.5.
-
-WARNING:
-This feature was [deprecated](https://gitlab.com/groups/gitlab-org/configure/-/epics/8) in GitLab 14.5.
-
-If you are deploying to a [Kubernetes cluster](../../user/infrastructure/clusters/index.md)
-associated with your project, you can configure these deployments from your
-`.gitlab-ci.yml` file.
-
-NOTE:
-Kubernetes configuration isn't supported for Kubernetes clusters
-[managed by GitLab](../../user/project/clusters/gitlab_managed_clusters.md).
-
-The following configuration options are supported:
-
-- [`namespace`](https://kubernetes.io/docs/concepts/overview/working-with-objects/namespaces/)
-
-In the following example, the job deploys your application to the
-`production` Kubernetes namespace.
-
-```yaml
-deploy:
- stage: deploy
- script:
- - echo "Deploy to production server"
- environment:
- name: production
- url: https://example.com
- kubernetes:
- namespace: production
- rules:
- - if: $CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH
-```
-
-When you use the GitLab Kubernetes integration to deploy to a Kubernetes cluster,
-you can view cluster and namespace information. On the deployment
-job page, it's displayed above the job trace:
-
-
-
-### Configure incremental rollouts
-
-Learn how to release production changes to only a portion of your Kubernetes pods with
-[incremental rollouts](../environments/incremental_rollouts.md).
-
## Track newly included merge requests per deployment
GitLab can track newly included merge requests per deployment.
@@ -971,14 +923,14 @@ the `review/feature-1` spec takes precedence over `review/*` and `*` specs.
## Related topics
-- [Use GitLab CI to deploy to multiple environments (blog post)](https://about.gitlab.com/blog/2021/02/05/ci-deployment-and-environments/)
-- [Review Apps](../review_apps/index.md): Use dynamic environments to deploy your code for every branch.
-- [Deploy boards](../../user/project/deploy_boards.md): View the status of your applications running on Kubernetes.
-- [Protected environments](protected_environments.md): Determine who can deploy code to your environments.
-- [Environments Dashboard](../environments/environments_dashboard.md): View a summary of each
- environment's operational health. **(PREMIUM)**
-- [Deployment safety](deployment_safety.md#restrict-write-access-to-a-critical-environment): Secure your deployments.
-- [Track deployments of an external deployment tool](external_deployment_tools.md): Use an external deployment tool instead of built-in deployment solution.
+- [Kubernetes dashboard](kubernetes_dashboard.md)
+- [Deploy to multiple environments with GitLab CI/CD (blog post)](https://about.gitlab.com/blog/2021/02/05/ci-deployment-and-environments/)
+- [Review Apps](../review_apps/index.md)
+- [Protected environments](protected_environments.md)
+- [Environments Dashboard](../environments/environments_dashboard.md)
+- [Deployment safety](deployment_safety.md#restrict-write-access-to-a-critical-environment)
+- [Track deployments of an external deployment tool](external_deployment_tools.md)
+- [Configure Kubernetes deployments (deprecated)](configure_kubernetes_deployments.md)
## Troubleshooting
diff --git a/doc/ci/environments/kubernetes_dashboard.md b/doc/ci/environments/kubernetes_dashboard.md
new file mode 100644
index 00000000000..c669898602f
--- /dev/null
+++ b/doc/ci/environments/kubernetes_dashboard.md
@@ -0,0 +1,69 @@
+---
+stage: Deploy
+group: Environments
+info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments
+type: reference
+---
+
+# Kubernetes Dashboard **(FREE)**
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/390769) in GitLab 16.1, with [flags](../../administration/feature_flags.md) named `environment_settings_to_graphql`, `kas_user_access`, `kas_user_access_project`, and `expose_authorized_cluster_agents`. Disabled by default.
+
+FLAG:
+On self-managed GitLab, by default this feature is not available. To make it available, ask an administrator to [enable the feature flags](../../administration/feature_flags.md) named `environment_settings_to_graphql`, `kas_user_access`, `kas_user_access_project`, and `expose_authorized_cluster_agents`.
+
+Use the Kubernetes Dashboard to understand the status of your clusters with an intuitive visual interface.
+The Kubernetes Dashboard works with every connected Kubernetes cluster, whether you deployed them
+with CI/CD or GitOps.
+
+For Flux users, the synchronization status of a given environment is not displayed in the dashboard.
+[Issue 391581](https://gitlab.com/gitlab-org/gitlab/-/issues/391581) proposes to add this functionality.
+
+## Configure the Kubernetes Dashboard
+
+Configure a Kubernetes Dashboard to use it for a given environment.
+You can configure dashboard for an environment that already exists, or
+add one when you create an environment.
+
+Prerequisite:
+
+- The agent for Kubernetes must be shared with the environment's project, or its parent group, using the [`user_access`](../../user/clusters/agent/user_access.md) keyword.
+
+### The environment already exists
+
+1. On the top bar, select **Main menu > Projects** and find your project.
+1. On the left sidebar, select **Deployments > Environments**.
+1. Select the environment to be associated with the Kubernetes.
+1. Select **Edit**.
+1. Select a GitLab agent for Kubernetes.
+1. Select **Save**.
+
+### The environment doesn't exist
+
+1. On the top bar, select **Main menu > Projects** and find your project.
+1. On the left sidebar, select **Deployments > Environments**.
+1. Select **New environment**.
+1. Complete the **Name** field.
+1. Select a GitLab agent for Kubernetes.
+1. Select **Save**.
+
+## View a dashboard
+
+To view a configured Kubernetes Dashboard:
+
+1. On the top bar, select **Main menu > Projects** and find your project.
+1. On the left sidebar, select **Deployments > Environments**.
+1. Expand the environment associated with GitLab agent for Kubernetes.
+1. Expand **Kubernetes overview**.
+
+## Troubleshooting
+
+When working with the Kubernetes Dashboard, you might encounter the following issues.
+
+### User cannot list resource in API group
+
+You might get an error that states `Error: services is forbidden: User "gitlab:user:" cannot list resource "" in API group "" at the cluster scope`.
+
+This error happens when a user is not allowed to do the specified operation in the [Kubernetes RBAC](https://kubernetes.io/docs/reference/access-authn-authz/rbac/).
+
+To resolve, check your [RBAC configuration](../../user/clusters/agent/user_access.md#configure-kubernetes-access). If the RBAC is properly configured, contact your Kubernetes administrator.
diff --git a/doc/ci/yaml/index.md b/doc/ci/yaml/index.md
index c966676cbc9..ec7ab8e3a28 100644
--- a/doc/ci/yaml/index.md
+++ b/doc/ci/yaml/index.md
@@ -1850,7 +1850,7 @@ environment, using the `production`
**Related topics**:
-- [Available settings for `kubernetes`](../environments/index.md#configure-kubernetes-deployments-deprecated).
+- [Available settings for `kubernetes`](../environments/configure_kubernetes_deployments.md).
#### `environment:deployment_tier`
diff --git a/doc/development/testing_guide/flaky_tests.md b/doc/development/testing_guide/flaky_tests.md
index 40be7a7d094..9114ec3d179 100644
--- a/doc/development/testing_guide/flaky_tests.md
+++ b/doc/development/testing_guide/flaky_tests.md
@@ -13,62 +13,48 @@ eventually.
## What are the potential cause for a test to be flaky?
-### Unclean environment
+### State leak
-**Label:** `flaky-test::unclean environment`
+**Label:** `flaky-test::state leak`
-**Description:** The environment got dirtied by a previous test. The actual cause is probably not the flaky test here.
+**Description:** Data state has leaked from a previous test. The actual cause is probably not the flaky test here.
**Difficulty to reproduce:** Moderate. Usually, running the same spec files until the one that's failing reproduces the problem.
-**Resolution:** Fix the previous tests and/or places where the environment is modified, so that
+**Resolution:** Fix the previous tests and/or places where the test data or environment is modified, so that
it's reset to a pristine test after each test.
**Examples:**
-- [Example 1](https://gitlab.com/gitlab-org/gitlab/-/issues/378414#note_1142026988): A migration
+- [Example 1](https://gitlab.com/gitlab-org/gitlab/-/issues/402915): State leakage can result from
+ data records created with `let_it_be` shared between test examples, while some test modifies the model
+ either deliberately or unwillingly causing out-of-sync data in test examples. This can result in `PG::QueryCanceled: ERROR` in the subsequent test examples or retries.
+ For more information about state leakages and resolution options, see [GitLab testing best practices](best_practices.md#lets-talk-about-let).
+- [Example 2](https://gitlab.com/gitlab-org/gitlab/-/issues/378414#note_1142026988): A migration
test might roll-back the database, perform its testing, and then roll-up the database in an
inconsistent state, so that following tests might not know about certain columns.
-- [Example 2](https://gitlab.com/gitlab-org/gitlab/-/issues/368500): A test modifies data that is
+- [Example 3](https://gitlab.com/gitlab-org/gitlab/-/issues/368500): A test modifies data that is
used by a following test.
-- [Example 3](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/103434#note_1172316521): A test for a database query passes in a fresh database, but in a
+- [Example 4](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/103434#note_1172316521): A test for a database query passes in a fresh database, but in a
CI/CD pipeline where the database is used to process previous test sequences, the test fails. This likely
- means that the query itself needs to be updated to work in a non-clean database.
-
-### Ordering assertion
-
-**Label:** `flaky-test::ordering assertion`
-
-**Description:** The test is expecting a specific order in the data under test yet the data is in
-a non-deterministic order.
-
-**Difficulty to reproduce:** Easy. Usually, running the test locally several times would reproduce
-the problem.
-
-**Resolution:** Depending on the problem, you might want to:
-
-- loosen the assertion if the test shouldn't care about ordering but only on the elements
-- fix the test by specifying a deterministic ordering
-- fix the app code by specifying a deterministic ordering
-
-**Examples:**
-
-- [Example 1](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/10148/diffs): Without
- specifying `ORDER BY`, database will not give deterministic ordering, or data race happening
- in the tests.
-- [Example 2](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/106936/diffs).
+ means that the query itself needs to be updated to work in a non-clean database.
### Dataset-specific
**Label:** `flaky-test::dataset-specific`
-**Description:** The test assumes the dataset is in a particular (usually limited) state, which
+**Description:** The test assumes the dataset is in a particular (usually limited) state or order, which
might not be true depending on when the test run during the test suite.
**Difficulty to reproduce:** Moderate, as the amount of data needed to reproduce the issue might be
-difficult to achieve locally.
+difficult to achieve locally. Ordering issues are easier to reproduce by repeatedly running the tests several times.
-**Resolution:** Fix the test to not assume that the dataset is in a particular state, don't hardcode IDs.
+**Resolution:**
+
+- Fix the test to not assume that the dataset is in a particular state, don't hardcode IDs.
+- Loosen the assertion if the test shouldn't care about ordering but only on the elements.
+- Fix the test by specifying a deterministic ordering.
+- Fix the app code by specifying a deterministic ordering.
**Examples:**
@@ -81,11 +67,10 @@ difficult to achieve locally.
suite, it might pass as not enough records were created before it, but as soon as it would run
later in the suite, there could be a record that actually has the ID `42`, hence the test would
start to fail.
-- [Example 3](https://gitlab.com/gitlab-org/gitlab/-/issues/402915): State leakage can result from
- data records created with `let_it_be` shared between test examples, while some test modifies the model
- either deliberately or unwillingly causing out-of-sync data in test examples. This can result in `PG::QueryCanceled: ERROR` in the subsequent test examples or retries.
- For more information about state leakages and resolution options,
- see [GitLab testing best practices](best_practices.md#lets-talk-about-let).
+- [Example 3](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/10148/diffs): Without
+ specifying `ORDER BY`, database is not given deterministic ordering, or data race can happen
+ in the tests.
+- [Example 4](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/106936/diffs).
### Random input
diff --git a/doc/user/clusters/agent/install/index.md b/doc/user/clusters/agent/install/index.md
index e91bcd6e330..12600747fad 100644
--- a/doc/user/clusters/agent/install/index.md
+++ b/doc/user/clusters/agent/install/index.md
@@ -29,7 +29,7 @@ Before you can install the agent in your cluster, you need:
To install the agent in your cluster:
-1. Optional. [Create an agent configuration file](#create-an-agent-configuration-file).
+1. [Create an agent configuration file](#create-an-agent-configuration-file).
1. [Register the agent with GitLab](#register-the-agent-with-gitlab).
1. [Install the agent in your cluster](#install-the-agent-in-the-cluster).
@@ -44,8 +44,7 @@ For configuration settings, the agent uses a YAML file in the GitLab project. Yo
- You use [a GitOps workflow](../gitops.md#gitops-workflow-steps).
- You use [a GitLab CI/CD workflow](../ci_cd_workflow.md#gitlab-cicd-workflow-steps) and want to authorize a different project to use the agent.
-
-Otherwise it is optional.
+- You [allow specific project or group members to access Kubernetes](../user_access.md).
To create an agent configuration file:
@@ -58,14 +57,12 @@ To create an agent configuration file:
- Start with an alphanumeric character.
- End with an alphanumeric character.
-1. In the repository, in the default branch, create this directory at the root:
+1. In the repository, in the default branch, create an agent configuration file at the root:
```plaintext
- .gitlab/agents/
+ .gitlab/agents//config.yaml
```
-1. In the directory, create a `config.yaml` file. Ensure the filename ends in `.yaml`, not `.yml`.
-
You can leave the file blank for now, and [configure it](#configure-your-agent) later.
### Register the agent with GitLab
diff --git a/doc/user/clusters/agent/user_access.md b/doc/user/clusters/agent/user_access.md
new file mode 100644
index 00000000000..f4c7b1f658c
--- /dev/null
+++ b/doc/user/clusters/agent/user_access.md
@@ -0,0 +1,155 @@
+---
+stage: Deploy
+group: Environments
+info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments
+---
+
+# Grant users Kubernetes access **(FREE)**
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/390769) in GitLab 16.1, with [flags](../../../administration/feature_flags.md) named `environment_settings_to_graphql`, `kas_user_access`, `kas_user_access_project`, and `expose_authorized_cluster_agents`. Disabled by default.
+
+FLAG:
+On self-managed GitLab, by default this feature is not available. To make it available, ask an administrator to [enable the feature flags](../../../administration/feature_flags.md) named `environment_settings_to_graphql`, `kas_user_access`, `kas_user_access_project`, and `expose_authorized_cluster_agents`.
+
+As an administrator of Kubernetes clusters in an organization, you can grant Kubernetes access to members
+of a specific project or group.
+
+Granting access also activates the Kubernetes Dashboard for a project or group.
+
+## Configure Kubernetes access
+
+Configure access when you want to grant users access
+to a Kubernetes cluster.
+
+Prerequisites:
+
+- The agent for Kubernetes is installed in the Kubernetes cluster.
+- You must have the Developer role or higher.
+
+To configure access:
+
+- In the agent configuration file, define a `user_access` keyword with the following parameters:
+
+ - `projects`: A list of projects whose members should have access.
+ - `groups`: A list of groups whose members should have access.
+ - `access_as`: Required. For plain access, the value is `{ agent: {...} }`.
+
+After you configure access, requests are forwarded to the API server using
+the agent service account.
+For example:
+
+```yaml
+# .gitlab/agents/my-agent/config.yaml
+
+user_access:
+ access_as:
+ agent: {}
+ projects:
+ - id: group-1/project-1
+ - id: group-2/project-2
+ groups:
+ - id: group-2
+ - id: group-3/subgroup
+```
+
+## Configure access with user impersonation **(PREMIUM)**
+
+You can grant access to a Kubernetes cluster and transform
+requests into impersonation requests for authenticated users.
+
+Prerequisites:
+
+- The agent for Kubernetes is installed in the Kubernetes cluster.
+- You must have the Developer role or higher.
+
+To configure access with user impersonation:
+
+- In the agent configuration file, define a `user_access` keyword with the following parameters:
+
+ - `projects`: A list of projects whose members should have access.
+ - `groups`: A list of groups whose members should have access.
+ - `access_as`: Required. For user impersonation, the value is `{ user: {...} }`.
+
+After you configure access, requests are transformed into impersonation requests for
+authenticated users.
+
+### User impersonation workflow
+
+The installed `agentk` impersonates the given users as follows:
+
+- `UserName` is `gitlab:user:`
+- `Groups` is:
+ - `gitlab:user`: Common to all requests coming from GitLab users.
+ - `gitlab:project_role::` for each role in each authorized project.
+ - `gitlab:group_role::` for each role in each authorized group.
+- `Extra` carries additional information about the request:
+ - `agent.gitlab.com/id`: The agent ID.
+ - `agent.gitlab.com/username`: The username of the GitLab user.
+ - `agent.gitlab.com/config_project_id`: The agent configuration project ID.
+ - `agent.gitlab.com/access_type`: One of `personal_access_token`,
+ `oidc_id_token`, or `session_cookie`.
+
+Only projects and groups directly listed in the under `user_access` in the configuration
+file are impersonated. For example:
+
+```yaml
+# .gitlab/agents/my-agent/config.yaml
+
+user_access:
+ access_as:
+ user: {}
+ projects:
+ - id: group-1/project-1 # group_id=1, project_id=1
+ - id: group-2/project-2 # group_id=2, project_id=2
+ groups:
+ - id: group-2 # group_id=2
+ - id: group-3/subgroup # group_id=3, group_id=4
+```
+
+In this configuration:
+
+- If a user is a member of only `group-1`, they receive
+ only the Kubernetes RBAC groups `gitlab:project_role:1:`.
+- If a user is a member of `group-2`, they receive both Kubernetes RBAC groups:
+ - `gitlab:project_role:2:`,
+ - `gitlab:group_role:2:`.
+
+### RBAC authorization
+
+Impersonated requests require `ClusterRoleBinding` or `RoleBinding` to identify the resource permissions
+inside Kubernetes. See [RBAC authorization](https://kubernetes.io/docs/reference/access-authn-authz/rbac/)
+for the appropriate configuration.
+
+For example, if you allow maintainers in `awesome-org/deployment` project (ID: 123) to read the Kubernetes workloads,
+you must add a `ClusterRoleBinding` resource to your Kubernetes configuration:
+
+```yaml
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRoleBinding
+metadata:
+ name: my-cluster-role-binding
+roleRef:
+ name: view
+ kind: ClusterRole
+ apiGroup: rbac.authorization.k8s.io
+subjects:
+ - name: gitlab:project_role:123:maintainer
+ kind: Group
+```
+
+## Related topics
+
+- [Architectural blueprint](https://gitlab.com/gitlab-org/cluster-integration/gitlab-agent/-/blob/master/doc/kubernetes_user_access.md)
+- [Kubernetes Dashboard](https://gitlab.com/groups/gitlab-org/-/epics/2493)
+
+
diff --git a/doc/user/clusters/agent/work_with_agent.md b/doc/user/clusters/agent/work_with_agent.md
index b2e8ac6ef16..2b69141d9c5 100644
--- a/doc/user/clusters/agent/work_with_agent.md
+++ b/doc/user/clusters/agent/work_with_agent.md
@@ -30,6 +30,22 @@ On this page, you can view:
- The version of `agentk` installed on your cluster.
- The path to each agent configuration file.
+## View shared agents
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/395498) in GitLab 16.1.
+
+In addition to the agents owned by your project, you can also view agents shared with the
+[`ci_access`](ci_cd_workflow.md) and [`user_access`](user_access.md) keywords. Once an agent
+is shared with a project, it automatically appears in the project agent tab.
+
+To view the list of shared agents:
+
+1. On the top bar, select **Main menu > Projects** and find the project.
+1. On the left sidebar, select **Infrastructure > Kubernetes clusters**.
+1. Select the **Agent** tab.
+
+The list of shared agents and their clusters are displayed.
+
## View an agent's activity information
> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/277323) in GitLab 14.6.
diff --git a/doc/user/project/clusters/deploy_to_cluster.md b/doc/user/project/clusters/deploy_to_cluster.md
index 31d5a73757a..2ea7ac0f1fd 100644
--- a/doc/user/project/clusters/deploy_to_cluster.md
+++ b/doc/user/project/clusters/deploy_to_cluster.md
@@ -78,7 +78,7 @@ You can customize the deployment namespace in a few ways:
- For **non-managed** clusters, the auto-generated namespace is set in the `KUBECONFIG`,
but the user is responsible for ensuring its existence. You can fully customize
this value using
- [`environment:kubernetes:namespace`](../../../ci/environments/index.md#configure-kubernetes-deployments-deprecated)
+ [`environment:kubernetes:namespace`](../../../ci/environments/configure_kubernetes_deployments.md)
in `.gitlab-ci.yml`.
When you customize the namespace, existing environments remain linked to their current
diff --git a/lib/api/ci/secure_files.rb b/lib/api/ci/secure_files.rb
index c08889b87a2..02f625f2130 100644
--- a/lib/api/ci/secure_files.rb
+++ b/lib/api/ci/secure_files.rb
@@ -6,6 +6,7 @@ module API
include PaginationParams
before do
+ check_api_enabled!
authenticate!
authorize! :read_secure_files, user_project
end
@@ -64,7 +65,7 @@ module API
resource do
before do
- read_only_feature_flag_enabled?
+ check_read_only_feature_flag_enabled!
authorize! :admin_secure_files, user_project
end
@@ -112,7 +113,11 @@ module API
end
helpers do
- def read_only_feature_flag_enabled?
+ def check_api_enabled!
+ forbidden! unless Gitlab.config.ci_secure_files.enabled
+ end
+
+ def check_read_only_feature_flag_enabled!
service_unavailable! if Feature.enabled?(:ci_secure_files_read_only, user_project, type: :ops)
end
end
diff --git a/lib/gitlab/diff/formatters/file_formatter.rb b/lib/gitlab/diff/formatters/file_formatter.rb
new file mode 100644
index 00000000000..37b9ad85ef8
--- /dev/null
+++ b/lib/gitlab/diff/formatters/file_formatter.rb
@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+module Gitlab
+ module Diff
+ module Formatters
+ class FileFormatter < BaseFormatter
+ def initialize(attrs)
+ @ignore_whitespace_change = false
+
+ super(attrs)
+ end
+
+ def key
+ @key ||= super.push(new_path, old_path)
+ end
+
+ def position_type
+ "file"
+ end
+
+ def complete?
+ [new_path, old_path].all?(&:present?)
+ end
+
+ def ==(other)
+ other.is_a?(self.class) &&
+ old_path == other.old_path &&
+ new_path == other.new_path
+ end
+ end
+ end
+ end
+end
diff --git a/lib/gitlab/diff/position.rb b/lib/gitlab/diff/position.rb
index cbb7231261b..feee4bcc7f9 100644
--- a/lib/gitlab/diff/position.rb
+++ b/lib/gitlab/diff/position.rb
@@ -150,6 +150,10 @@ module Gitlab
@file_hash ||= Digest::SHA1.hexdigest(file_path)
end
+ def on_file?
+ position_type == 'file'
+ end
+
def on_image?
position_type == 'image'
end
@@ -185,6 +189,8 @@ module Gitlab
case type
when 'image'
Gitlab::Diff::Formatters::ImageFormatter
+ when 'file'
+ Gitlab::Diff::Formatters::FileFormatter
else
Gitlab::Diff::Formatters::TextFormatter
end
diff --git a/lib/gitlab/diff/position_tracer.rb b/lib/gitlab/diff/position_tracer.rb
index faa8a46e6ed..a8c0108fa34 100644
--- a/lib/gitlab/diff/position_tracer.rb
+++ b/lib/gitlab/diff/position_tracer.rb
@@ -22,9 +22,8 @@ module Gitlab
return unless old_position.diff_refs == old_diff_refs
@ignore_whitespace_change = old_position.ignore_whitespace_change
- strategy = old_position.on_text? ? LineStrategy : ImageStrategy
- strategy.new(self).trace(old_position)
+ strategy(old_position).new(self).trace(old_position)
end
def ac_diffs
@@ -49,6 +48,16 @@ module Gitlab
private
+ def strategy(old_position)
+ if old_position.on_text?
+ LineStrategy
+ elsif old_position.on_file?
+ FileStrategy
+ else
+ ImageStrategy
+ end
+ end
+
def compare(start_sha, head_sha, straight: false)
compare = CompareService.new(project, head_sha).execute(project, start_sha, straight: straight)
compare.diffs(paths: paths, expanded: true, ignore_whitespace_change: @ignore_whitespace_change)
diff --git a/lib/gitlab/diff/position_tracer/file_strategy.rb b/lib/gitlab/diff/position_tracer/file_strategy.rb
new file mode 100644
index 00000000000..171d78bf46f
--- /dev/null
+++ b/lib/gitlab/diff/position_tracer/file_strategy.rb
@@ -0,0 +1,47 @@
+# frozen_string_literal: true
+
+module Gitlab
+ module Diff
+ class PositionTracer
+ class FileStrategy < BaseStrategy
+ def trace(position)
+ a_path = position.old_path
+ b_path = position.new_path
+
+ # If file exists in B->D (e.g. updated, renamed, removed), let the
+ # note become outdated.
+ bd_diff = bd_diffs.diff_file_with_old_path(b_path)
+
+ return { position: new_position(position, bd_diff), outdated: true } if bd_diff
+
+ # If file still exists in the new diff, update the position.
+ cd_diff = cd_diffs.diff_file_with_new_path(b_path)
+
+ return { position: new_position(position, cd_diff), outdated: false } if cd_diff
+
+ # If file exists in A->C (e.g. rebased and same changes were present
+ # in target branch), let the note become outdated.
+ ac_diff = ac_diffs.diff_file_with_old_path(a_path)
+
+ return { position: new_position(position, ac_diff), outdated: true } if ac_diff
+
+ # If ever there's a case that the file no longer exists in any diff,
+ # don't set a change position and let the note become outdated.
+ #
+ # This should never happen given the file should exist in one of the
+ # diffs above.
+ { outdated: true }
+ end
+
+ private
+
+ def new_position(position, diff_file)
+ Position.new(
+ diff_file: diff_file,
+ position_type: position.position_type
+ )
+ end
+ end
+ end
+ end
+end
diff --git a/lib/gitlab/diff/position_tracer/image_strategy.rb b/lib/gitlab/diff/position_tracer/image_strategy.rb
index aac52b536f7..172eba4acd3 100644
--- a/lib/gitlab/diff/position_tracer/image_strategy.rb
+++ b/lib/gitlab/diff/position_tracer/image_strategy.rb
@@ -3,36 +3,7 @@
module Gitlab
module Diff
class PositionTracer
- class ImageStrategy < BaseStrategy
- def trace(position)
- a_path = position.old_path
- b_path = position.new_path
-
- # If file exists in B->D (e.g. updated, renamed, removed), let the
- # note become outdated.
- bd_diff = bd_diffs.diff_file_with_old_path(b_path)
-
- return { position: new_position(position, bd_diff), outdated: true } if bd_diff
-
- # If file still exists in the new diff, update the position.
- cd_diff = cd_diffs.diff_file_with_new_path(b_path)
-
- return { position: new_position(position, cd_diff), outdated: false } if cd_diff
-
- # If file exists in A->C (e.g. rebased and same changes were present
- # in target branch), let the note become outdated.
- ac_diff = ac_diffs.diff_file_with_old_path(a_path)
-
- return { position: new_position(position, ac_diff), outdated: true } if ac_diff
-
- # If ever there's a case that the file no longer exists in any diff,
- # don't set a change position and let the note become outdated.
- #
- # This should never happen given the file should exist in one of the
- # diffs above.
- { outdated: true }
- end
-
+ class ImageStrategy < FileStrategy
private
def new_position(position, diff_file)
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 7de8be43b77..ef88678525a 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -11207,6 +11207,9 @@ msgstr ""
msgid "Comment on lines %{startLine} to %{endLine}"
msgstr ""
+msgid "Comment on this file"
+msgstr ""
+
msgid "Comment template actions"
msgstr ""
@@ -28503,6 +28506,12 @@ msgstr ""
msgid "MergeRequests|started a thread"
msgstr ""
+msgid "MergeRequests|started a thread on %{linkStart}a file%{linkEnd}"
+msgstr ""
+
+msgid "MergeRequests|started a thread on %{linkStart}an old version of a file%{linkEnd}"
+msgstr ""
+
msgid "MergeRequests|started a thread on %{linkStart}an old version of the diff%{linkEnd}"
msgstr ""
diff --git a/qa/qa/page/main/login.rb b/qa/qa/page/main/login.rb
index 245f0a189cc..d79d2766248 100644
--- a/qa/qa/page/main/login.rb
+++ b/qa/qa/page/main/login.rb
@@ -109,6 +109,7 @@ module QA
# Happens on clean GDK installations when seeded root admin password is expired
#
def set_up_new_password_if_required(user:, skip_page_validation:)
+ Support::WaitForRequests.wait_for_requests
return unless has_content?('Set up new password', wait: 1)
Profile::Password.perform do |new_password_page|
diff --git a/spec/config/object_store_settings_spec.rb b/spec/config/object_store_settings_spec.rb
index 0689dc130d9..14995e2934e 100644
--- a/spec/config/object_store_settings_spec.rb
+++ b/spec/config/object_store_settings_spec.rb
@@ -25,7 +25,6 @@ RSpec.describe ObjectStoreSettings, feature_category: :shared do
'artifacts' => { 'enabled' => true },
'external_diffs' => { 'enabled' => false },
'pages' => { 'enabled' => true },
- 'ci_secure_files' => { 'enabled' => true },
'object_store' => {
'enabled' => true,
'connection' => connection,
@@ -101,14 +100,6 @@ RSpec.describe ObjectStoreSettings, feature_category: :shared do
expect(settings.external_diffs['enabled']).to be false
expect(settings.external_diffs['object_store']).to be_nil
expect(settings.external_diffs).to eq(settings['external_diffs'])
-
- expect(settings.ci_secure_files['enabled']).to be true
- expect(settings.ci_secure_files['object_store']['enabled']).to be true
- expect(settings.ci_secure_files['object_store']['connection'].to_hash).to eq(connection)
- expect(settings.ci_secure_files['object_store']['remote_directory']).to eq('ci_secure_files')
- expect(settings.ci_secure_files['object_store']['bucket_prefix']).to eq(nil)
- expect(settings.ci_secure_files['object_store']['consolidated_settings']).to be true
- expect(settings.ci_secure_files).to eq(settings['ci_secure_files'])
end
it 'supports bucket prefixes' do
diff --git a/spec/features/merge_request/user_creates_discussion_on_diff_file_spec.rb b/spec/features/merge_request/user_creates_discussion_on_diff_file_spec.rb
new file mode 100644
index 00000000000..bb41ea6f6ed
--- /dev/null
+++ b/spec/features/merge_request/user_creates_discussion_on_diff_file_spec.rb
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'User creates discussion on diff file', :js, feature_category: :code_review_workflow do
+ let_it_be(:user) { create(:user) }
+ let_it_be(:project) { create(:project, :public, :repository) }
+ let_it_be(:merge_request) do
+ create(:merge_request_with_diffs, source_project: project, target_project: project, source_branch: 'merge-test')
+ end
+
+ before do
+ project.add_maintainer(user)
+ sign_in(user)
+
+ visit(diffs_project_merge_request_path(project, merge_request))
+ end
+
+ it 'creates discussion on diff file' do
+ first('.diff-file [data-testid="comment-files-button"]').click
+
+ send_keys "Test comment"
+
+ click_button "Add comment now"
+
+ expect(first('.diff-file')).to have_selector('.note-text', text: 'Test comment')
+ end
+end
diff --git a/spec/features/projects/settings/secure_files_spec.rb b/spec/features/projects/settings/secure_files_spec.rb
index 8ac64287fcb..7ff1a5f3568 100644
--- a/spec/features/projects/settings/secure_files_spec.rb
+++ b/spec/features/projects/settings/secure_files_spec.rb
@@ -12,6 +12,17 @@ RSpec.describe 'Secure Files', :js, feature_category: :groups_and_projects do
sign_in(user)
end
+ context 'when disabled at the instance level' do
+ before do
+ stub_config(ci_secure_files: { enabled: false })
+ end
+
+ it 'does not show the secure files settings' do
+ visit project_settings_ci_cd_path(project)
+ expect(page).not_to have_content('Secure Files')
+ end
+ end
+
context 'authenticated user with admin permissions' do
it 'shows the secure files settings' do
visit project_settings_ci_cd_path(project)
diff --git a/spec/frontend/batch_comments/components/diff_file_drafts_spec.js b/spec/frontend/batch_comments/components/diff_file_drafts_spec.js
index f667ebc0fcb..014e28b7509 100644
--- a/spec/frontend/batch_comments/components/diff_file_drafts_spec.js
+++ b/spec/frontend/batch_comments/components/diff_file_drafts_spec.js
@@ -16,7 +16,10 @@ describe('Batch comments diff file drafts component', () => {
batchComments: {
namespaced: true,
getters: {
- draftsForFile: () => () => [{ id: 1 }, { id: 2 }],
+ draftsForFile: () => () => [
+ { id: 1, position: { position_type: 'file' } },
+ { id: 2, position: { position_type: 'file' } },
+ ],
},
},
},
@@ -24,7 +27,7 @@ describe('Batch comments diff file drafts component', () => {
vm = shallowMount(DiffFileDrafts, {
store,
- propsData: { fileHash: 'filehash' },
+ propsData: { fileHash: 'filehash', positionType: 'file' },
});
}
diff --git a/spec/frontend/diffs/components/diff_content_spec.js b/spec/frontend/diffs/components/diff_content_spec.js
index fb271433762..39d9255aaf9 100644
--- a/spec/frontend/diffs/components/diff_content_spec.js
+++ b/spec/frontend/diffs/components/diff_content_spec.js
@@ -176,7 +176,12 @@ describe('DiffContent', () => {
getCommentFormForDiffFileGetterMock.mockReturnValue(() => true);
createComponent({
props: {
- diffFile: { ...imageDiffFile, discussions: [{ name: 'discussion-stub ' }] },
+ diffFile: {
+ ...imageDiffFile,
+ discussions: [
+ { name: 'discussion-stub', position: { position_type: IMAGE_DIFF_POSITION_TYPE } },
+ ],
+ },
},
});
@@ -186,7 +191,12 @@ describe('DiffContent', () => {
it('emits saveDiffDiscussion when note-form emits `handleFormUpdate`', () => {
const noteStub = {};
getCommentFormForDiffFileGetterMock.mockReturnValue(() => true);
- const currentDiffFile = { ...imageDiffFile, discussions: [{ name: 'discussion-stub ' }] };
+ const currentDiffFile = {
+ ...imageDiffFile,
+ discussions: [
+ { name: 'discussion-stub', position: { position_type: IMAGE_DIFF_POSITION_TYPE } },
+ ],
+ };
createComponent({
props: {
diffFile: currentDiffFile,
diff --git a/spec/frontend/diffs/components/diff_file_header_spec.js b/spec/frontend/diffs/components/diff_file_header_spec.js
index 900aa8d1469..3f75b086368 100644
--- a/spec/frontend/diffs/components/diff_file_header_spec.js
+++ b/spec/frontend/diffs/components/diff_file_header_spec.js
@@ -18,7 +18,10 @@ import ClipboardButton from '~/vue_shared/components/clipboard_button.vue';
import testAction from '../../__helpers__/vuex_action_helper';
import diffDiscussionsMockData from '../mock_data/diff_discussions';
-jest.mock('~/lib/utils/common_utils');
+jest.mock('~/lib/utils/common_utils', () => ({
+ scrollToElement: jest.fn(),
+ isLoggedIn: () => true,
+}));
const diffFile = Object.freeze(
Object.assign(diffDiscussionsMockData.diff_file, {
@@ -47,6 +50,9 @@ describe('DiffFileHeader component', () => {
const diffHasDiscussionsResultMock = jest.fn();
const defaultMockStoreConfig = {
state: {},
+ getters: {
+ getNoteableData: () => ({ current_user: { can_create_note: true } }),
+ },
modules: {
diffs: {
namespaced: true,
@@ -637,4 +643,23 @@ describe('DiffFileHeader component', () => {
},
);
});
+
+ it.each`
+ commentOnFiles | exists | existsText
+ ${false} | ${false} | ${'does not'}
+ ${true} | ${true} | ${'does'}
+ `(
+ '$existsText render comment on files button when commentOnFiles is $commentOnFiles',
+ ({ commentOnFiles, exists }) => {
+ window.gon = { current_user_id: 1 };
+ createComponent({
+ props: {
+ addMergeRequestButtons: true,
+ },
+ options: { provide: { glFeatures: { commentOnFiles } } },
+ });
+
+ expect(wrapper.find('[data-testid="comment-files-button"]').exists()).toEqual(exists);
+ },
+ );
});
diff --git a/spec/frontend/diffs/components/diff_file_spec.js b/spec/frontend/diffs/components/diff_file_spec.js
index 389b192a515..d9c57ed1470 100644
--- a/spec/frontend/diffs/components/diff_file_spec.js
+++ b/spec/frontend/diffs/components/diff_file_spec.js
@@ -553,4 +553,69 @@ describe('DiffFile', () => {
expect(wrapper.find('[data-testid="conflictsAlert"]').exists()).toBe(true);
});
});
+
+ describe('file discussions', () => {
+ it.each`
+ extraProps | exists | existsText
+ ${{}} | ${false} | ${'does not'}
+ ${{ hasCommentForm: false }} | ${false} | ${'does not'}
+ ${{ hasCommentForm: true }} | ${true} | ${'does'}
+ ${{ discussions: [{ id: 1, position: { position_type: 'file' } }] }} | ${true} | ${'does'}
+ ${{ drafts: [{ id: 1 }] }} | ${true} | ${'does'}
+ `(
+ 'discussions wrapper $existsText exist for file with $extraProps',
+ ({ extraProps, exists }) => {
+ const file = {
+ ...getReadableFile(),
+ ...extraProps,
+ };
+
+ ({ wrapper, store } = createComponent({
+ file,
+ options: { provide: { glFeatures: { commentOnFiles: true } } },
+ }));
+
+ expect(wrapper.find('[data-testid="file-discussions"]').exists()).toEqual(exists);
+ },
+ );
+
+ it.each`
+ hasCommentForm | exists | existsText
+ ${false} | ${false} | ${'does not'}
+ ${true} | ${true} | ${'does'}
+ `(
+ 'comment form $existsText exist for hasCommentForm with $hasCommentForm',
+ ({ hasCommentForm, exists }) => {
+ const file = {
+ ...getReadableFile(),
+ hasCommentForm,
+ };
+
+ ({ wrapper, store } = createComponent({
+ file,
+ options: { provide: { glFeatures: { commentOnFiles: true } } },
+ }));
+
+ expect(wrapper.find('[data-testid="file-note-form"]').exists()).toEqual(exists);
+ },
+ );
+
+ it.each`
+ discussions | exists | existsText
+ ${[]} | ${false} | ${'does not'}
+ ${[{ id: 1, position: { position_type: 'file' } }]} | ${true} | ${'does'}
+ `('discussions $existsText exist for $discussions', ({ discussions, exists }) => {
+ const file = {
+ ...getReadableFile(),
+ discussions,
+ };
+
+ ({ wrapper, store } = createComponent({
+ file,
+ options: { provide: { glFeatures: { commentOnFiles: true } } },
+ }));
+
+ expect(wrapper.find('[data-testid="diff-file-discussions"]').exists()).toEqual(exists);
+ });
+ });
});
diff --git a/spec/frontend/diffs/mock_data/diff_file.js b/spec/frontend/diffs/mock_data/diff_file.js
index e0e5778e0d5..eef68100378 100644
--- a/spec/frontend/diffs/mock_data/diff_file.js
+++ b/spec/frontend/diffs/mock_data/diff_file.js
@@ -334,5 +334,6 @@ export const getDiffFileMock = () => ({
},
],
discussions: [],
+ drafts: [],
renderingLines: false,
});
diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js
index f0061ad88d7..7534fe741e7 100644
--- a/spec/frontend/diffs/store/actions_spec.js
+++ b/spec/frontend/diffs/store/actions_spec.js
@@ -1889,4 +1889,28 @@ describe('DiffsStoreActions', () => {
},
);
});
+
+ describe('toggleFileCommentForm', () => {
+ it('commits TOGGLE_FILE_COMMENT_FORM', () => {
+ return testAction(
+ diffActions.toggleFileCommentForm,
+ 'path',
+ {},
+ [{ type: types.TOGGLE_FILE_COMMENT_FORM, payload: 'path' }],
+ [],
+ );
+ });
+ });
+
+ describe('addDraftToFile', () => {
+ it('commits ADD_DRAFT_TO_FILE', () => {
+ return testAction(
+ diffActions.addDraftToFile,
+ { filePath: 'path', draft: 'draft' },
+ {},
+ [{ type: types.ADD_DRAFT_TO_FILE, payload: { filePath: 'path', draft: 'draft' } }],
+ [],
+ );
+ });
+ });
});
diff --git a/spec/frontend/diffs/store/getters_spec.js b/spec/frontend/diffs/store/getters_spec.js
index ed7b6699e2c..8097f0976f6 100644
--- a/spec/frontend/diffs/store/getters_spec.js
+++ b/spec/frontend/diffs/store/getters_spec.js
@@ -188,6 +188,24 @@ describe('Diffs Module Getters', () => {
expect(getters.diffHasExpandedDiscussions(localState)(diffFile)).toEqual(true);
});
+ it('returns true when file discussion is expanded', () => {
+ const diffFile = {
+ discussions: [{ ...discussionMock, expanded: true }],
+ highlighted_diff_lines: [],
+ };
+
+ expect(getters.diffHasExpandedDiscussions(localState)(diffFile)).toEqual(true);
+ });
+
+ it('returns false when file discussion is expanded', () => {
+ const diffFile = {
+ discussions: [{ ...discussionMock, expanded: false }],
+ highlighted_diff_lines: [],
+ };
+
+ expect(getters.diffHasExpandedDiscussions(localState)(diffFile)).toEqual(false);
+ });
+
it('returns false when there are no discussions', () => {
const diffFile = {
parallel_diff_lines: [],
@@ -231,6 +249,15 @@ describe('Diffs Module Getters', () => {
expect(getters.diffHasDiscussions(localState)(diffFile)).toEqual(true);
});
+ it('returns true when file has discussions', () => {
+ const diffFile = {
+ discussions: [discussionMock, discussionMock],
+ highlighted_diff_lines: [],
+ };
+
+ expect(getters.diffHasDiscussions(localState)(diffFile)).toEqual(true);
+ });
+
it('returns false when getDiffFileDiscussions returns no discussions', () => {
const diffFile = {
parallel_diff_lines: [],
diff --git a/spec/frontend/diffs/store/mutations_spec.js b/spec/frontend/diffs/store/mutations_spec.js
index ed8d7397bbc..b089cf22b14 100644
--- a/spec/frontend/diffs/store/mutations_spec.js
+++ b/spec/frontend/diffs/store/mutations_spec.js
@@ -269,6 +269,53 @@ describe('DiffsStoreMutations', () => {
expect(state.diffFiles[0][INLINE_DIFF_LINES_KEY][0].discussions[0].id).toEqual(1);
});
+ it('should add discussions to the given file', () => {
+ const diffPosition = {
+ base_sha: 'ed13df29948c41ba367caa757ab3ec4892509910',
+ head_sha: 'b921914f9a834ac47e6fd9420f78db0f83559130',
+ new_line: null,
+ new_path: '500-lines-4.txt',
+ old_line: 5,
+ old_path: '500-lines-4.txt',
+ start_sha: 'ed13df29948c41ba367caa757ab3ec4892509910',
+ type: 'file',
+ };
+
+ const state = {
+ latestDiff: true,
+ diffFiles: [
+ {
+ file_hash: 'ABC',
+ [INLINE_DIFF_LINES_KEY]: [],
+ discussions: [],
+ },
+ ],
+ };
+ const discussion = {
+ id: 1,
+ line_code: 'ABC_1',
+ diff_discussion: true,
+ resolvable: true,
+ original_position: diffPosition,
+ position: diffPosition,
+ diff_file: {
+ file_hash: state.diffFiles[0].file_hash,
+ },
+ };
+
+ const diffPositionByLineCode = {
+ ABC_1: diffPosition,
+ };
+
+ mutations[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, {
+ discussion,
+ diffPositionByLineCode,
+ });
+
+ expect(state.diffFiles[0].discussions.length).toEqual(1);
+ expect(state.diffFiles[0].discussions[0].id).toEqual(1);
+ });
+
it('should not duplicate discussions on line', () => {
const diffPosition = {
base_sha: 'ed13df29948c41ba367caa757ab3ec4892509910',
@@ -957,4 +1004,25 @@ describe('DiffsStoreMutations', () => {
expect(state.mrReviews).toStrictEqual(newReviews);
});
});
+
+ describe('TOGGLE_FILE_COMMENT_FORM', () => {
+ it('toggles diff files hasCommentForm', () => {
+ const state = { diffFiles: [{ file_path: 'path', hasCommentForm: false }] };
+
+ mutations[types.TOGGLE_FILE_COMMENT_FORM](state, 'path');
+
+ expect(state.diffFiles[0].hasCommentForm).toEqual(true);
+ });
+ });
+
+ describe('ADD_DRAFT_TO_FILE', () => {
+ it('adds draft to diff file', () => {
+ const state = { diffFiles: [{ file_path: 'path', drafts: [] }] };
+
+ mutations[types.ADD_DRAFT_TO_FILE](state, { filePath: 'path', draft: 'test' });
+
+ expect(state.diffFiles[0].drafts.length).toEqual(1);
+ expect(state.diffFiles[0].drafts[0]).toEqual('test');
+ });
+ });
});
diff --git a/spec/helpers/ci/secure_files_helper_spec.rb b/spec/helpers/ci/secure_files_helper_spec.rb
index 54307e670e1..049a09afd03 100644
--- a/spec/helpers/ci/secure_files_helper_spec.rb
+++ b/spec/helpers/ci/secure_files_helper_spec.rb
@@ -2,7 +2,7 @@
require 'spec_helper'
-RSpec.describe Ci::SecureFilesHelper do
+RSpec.describe Ci::SecureFilesHelper, feature_category: :mobile_devops do
let_it_be(:maintainer) { create(:user) }
let_it_be(:developer) { create(:user) }
let_it_be(:guest) { create(:user) }
@@ -19,6 +19,16 @@ RSpec.describe Ci::SecureFilesHelper do
subject { helper.show_secure_files_setting(project, user) }
describe '#show_secure_files_setting' do
+ context 'when disabled at the instance level' do
+ before do
+ stub_config(ci_secure_files: { enabled: false })
+ end
+
+ let(:user) { maintainer }
+
+ it { is_expected.to be false }
+ end
+
context 'authenticated user with admin permissions' do
let(:user) { maintainer }
diff --git a/spec/lib/gitlab/diff/formatters/file_formatter_spec.rb b/spec/lib/gitlab/diff/formatters/file_formatter_spec.rb
new file mode 100644
index 00000000000..32e5f17f7eb
--- /dev/null
+++ b/spec/lib/gitlab/diff/formatters/file_formatter_spec.rb
@@ -0,0 +1,44 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Diff::Formatters::FileFormatter, feature_category: :code_review_workflow do
+ let(:base_attrs) do
+ {
+ base_sha: 123,
+ start_sha: 456,
+ head_sha: 789,
+ old_path: nil,
+ new_path: nil,
+ position_type: 'file'
+ }
+ end
+
+ let(:attrs) { base_attrs.merge(old_path: 'path.rb', new_path: 'path.rb') }
+
+ it_behaves_like 'position formatter' do
+ # rubocop:disable Fips/SHA1 (This is used to match the existing class method)
+ let(:key) do
+ [123, 456, 789,
+ Digest::SHA1.hexdigest(formatter.old_path), Digest::SHA1.hexdigest(formatter.new_path),
+ 'path.rb', 'path.rb']
+ end
+ # rubocop:enable Fips/SHA1
+ end
+
+ describe '#==' do
+ subject { described_class.new(attrs) }
+
+ it { is_expected.to eq(subject) }
+
+ [:old_path, :new_path].each do |attr|
+ context "with attribute:#{attr}" do
+ let(:other_formatter) do
+ described_class.new(attrs.merge(attr => 9))
+ end
+
+ it { is_expected.not_to eq(other_formatter) }
+ end
+ end
+ end
+end
diff --git a/spec/lib/gitlab/diff/position_tracer/file_strategy_spec.rb b/spec/lib/gitlab/diff/position_tracer/file_strategy_spec.rb
new file mode 100644
index 00000000000..0d03f7ce6ca
--- /dev/null
+++ b/spec/lib/gitlab/diff/position_tracer/file_strategy_spec.rb
@@ -0,0 +1,238 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Diff::PositionTracer::FileStrategy, feature_category: :code_review_workflow do
+ include PositionTracerHelpers
+
+ let_it_be(:project) { create(:project, :repository) }
+ let(:current_user) { project.first_owner }
+ let(:file_name) { 'test-file' }
+ let(:new_file_name) { "#{file_name}-new" }
+ let(:second_file_name) { "#{file_name}-2" }
+ let(:branch_name) { 'position-tracer-test' }
+ let(:old_position) { position(old_path: file_name, new_path: file_name, position_type: 'file') }
+
+ let(:tracer) do
+ Gitlab::Diff::PositionTracer.new(
+ project: project,
+ old_diff_refs: old_diff_refs,
+ new_diff_refs: new_diff_refs
+ )
+ end
+
+ let(:strategy) { described_class.new(tracer) }
+
+ let(:initial_commit) do
+ project.commit(create_branch(branch_name, 'master')[:branch]&.name || 'master')
+ end
+
+ subject { strategy.trace(old_position) }
+
+ describe '#trace' do
+ describe 'diff scenarios' do
+ let(:create_file_commit) do
+ initial_commit
+
+ create_file(
+ branch_name,
+ file_name,
+ Base64.encode64('content')
+ )
+ end
+
+ let(:update_file_commit) do
+ create_file_commit
+
+ update_file(
+ branch_name,
+ file_name,
+ Base64.encode64('updatedcontent')
+ )
+ end
+
+ let(:update_file_again_commit) do
+ update_file_commit
+
+ update_file(
+ branch_name,
+ file_name,
+ Base64.encode64('updatedcontentagain')
+ )
+ end
+
+ let(:delete_file_commit) do
+ create_file_commit
+ delete_file(branch_name, file_name)
+ end
+
+ let(:rename_file_commit) do
+ delete_file_commit
+
+ create_file(
+ branch_name,
+ new_file_name,
+ Base64.encode64('renamedcontent')
+ )
+ end
+
+ let(:create_second_file_commit) do
+ create_file_commit
+
+ create_file(
+ branch_name,
+ second_file_name,
+ Base64.encode64('morecontent')
+ )
+ end
+
+ let(:create_another_file_commit) do
+ create_file(
+ branch_name,
+ second_file_name,
+ Base64.encode64('morecontent')
+ )
+ end
+
+ let(:update_another_file_commit) do
+ update_file(
+ branch_name,
+ second_file_name,
+ Base64.encode64('updatedmorecontent')
+ )
+ end
+
+ context 'when the file was created in the old diff' do
+ context 'when the file is unchanged between the old and the new diff' do
+ let(:old_diff_refs) { diff_refs(initial_commit, create_file_commit) }
+ let(:new_diff_refs) { diff_refs(initial_commit, create_second_file_commit) }
+
+ it 'returns the new position' do
+ expect_new_position(
+ old_path: file_name,
+ new_path: file_name
+ )
+ end
+ end
+
+ context 'when the file was updated between the old and the new diff' do
+ let(:old_diff_refs) { diff_refs(initial_commit, create_file_commit) }
+ let(:new_diff_refs) { diff_refs(initial_commit, update_file_commit) }
+ let(:change_diff_refs) { diff_refs(create_file_commit, update_file_commit) }
+
+ it 'returns the position of the change' do
+ expect_change_position(
+ old_path: file_name,
+ new_path: file_name
+ )
+ end
+ end
+
+ context 'when the file was renamed in between the old and the new diff' do
+ let(:old_diff_refs) { diff_refs(initial_commit, create_file_commit) }
+ let(:new_diff_refs) { diff_refs(initial_commit, rename_file_commit) }
+ let(:change_diff_refs) { diff_refs(create_file_commit, rename_file_commit) }
+
+ it 'returns the position of the change' do
+ expect_change_position(
+ old_path: file_name,
+ new_path: file_name
+ )
+ end
+ end
+
+ context 'when the file was removed in between the old and the new diff' do
+ let(:old_diff_refs) { diff_refs(initial_commit, create_file_commit) }
+ let(:new_diff_refs) { diff_refs(initial_commit, delete_file_commit) }
+ let(:change_diff_refs) { diff_refs(create_file_commit, delete_file_commit) }
+
+ it 'returns the position of the change' do
+ expect_change_position(
+ old_path: file_name,
+ new_path: file_name
+ )
+ end
+ end
+
+ context 'when the file is unchanged in the new diff' do
+ let(:old_diff_refs) { diff_refs(initial_commit, create_file_commit) }
+ let(:new_diff_refs) { diff_refs(create_another_file_commit, update_another_file_commit) }
+ let(:change_diff_refs) { diff_refs(initial_commit, create_another_file_commit) }
+
+ it 'returns the position of the change' do
+ expect_change_position(
+ old_path: file_name,
+ new_path: file_name
+ )
+ end
+ end
+ end
+
+ context 'when the file was changed in the old diff' do
+ context 'when the file is unchanged in between the old and the new diff' do
+ let(:old_diff_refs) { diff_refs(create_file_commit, update_file_commit) }
+ let(:new_diff_refs) { diff_refs(create_file_commit, create_second_file_commit) }
+
+ it 'returns the new position' do
+ expect_new_position(
+ old_path: file_name,
+ new_path: file_name
+ )
+ end
+ end
+
+ context 'when the file was updated in between the old and the new diff' do
+ let(:old_diff_refs) { diff_refs(create_file_commit, update_file_commit) }
+ let(:new_diff_refs) { diff_refs(create_file_commit, update_file_again_commit) }
+ let(:change_diff_refs) { diff_refs(update_file_commit, update_file_again_commit) }
+
+ it 'returns the position of the change' do
+ expect_change_position(
+ old_path: file_name,
+ new_path: file_name
+ )
+ end
+ end
+
+ context 'when the file was renamed in between the old and the new diff' do
+ let(:old_diff_refs) { diff_refs(create_file_commit, update_file_commit) }
+ let(:new_diff_refs) { diff_refs(create_file_commit, rename_file_commit) }
+ let(:change_diff_refs) { diff_refs(update_file_commit, rename_file_commit) }
+
+ it 'returns the position of the change' do
+ expect_change_position(
+ old_path: file_name,
+ new_path: file_name
+ )
+ end
+ end
+
+ context 'when the file was removed in between the old and the new diff' do
+ let(:old_diff_refs) { diff_refs(create_file_commit, update_file_commit) }
+ let(:new_diff_refs) { diff_refs(create_file_commit, delete_file_commit) }
+ let(:change_diff_refs) { diff_refs(update_file_commit, delete_file_commit) }
+
+ it 'returns the position of the change' do
+ expect_change_position(
+ old_path: file_name,
+ new_path: file_name
+ )
+ end
+ end
+
+ context 'when the file is unchanged in the new diff' do
+ let(:old_diff_refs) { diff_refs(create_file_commit, update_file_commit) }
+ let(:new_diff_refs) { diff_refs(create_another_file_commit, update_another_file_commit) }
+ let(:change_diff_refs) { diff_refs(create_file_commit, create_another_file_commit) }
+
+ it 'returns the position of the change' do
+ expect_change_position(
+ old_path: file_name,
+ new_path: file_name
+ )
+ end
+ end
+ end
+ end
+ end
+end
diff --git a/spec/lib/gitlab/diff/position_tracer_spec.rb b/spec/lib/gitlab/diff/position_tracer_spec.rb
index 8831cee690d..4aa4f160fc9 100644
--- a/spec/lib/gitlab/diff/position_tracer_spec.rb
+++ b/spec/lib/gitlab/diff/position_tracer_spec.rb
@@ -18,8 +18,13 @@ RSpec.describe Gitlab::Diff::PositionTracer do
let(:project) { double }
let(:old_diff_refs) { diff_refs }
let(:new_diff_refs) { diff_refs }
- let(:position) { double(on_text?: on_text?, diff_refs: diff_refs, ignore_whitespace_change: false) }
+ let(:on_file?) { false }
+ let(:on_text?) { false }
let(:tracer) { double }
+ let(:position) do
+ double(on_text?: on_text?, on_image?: false, on_file?: on_file?, diff_refs: diff_refs,
+ ignore_whitespace_change: false)
+ end
context 'position is on text' do
let(:on_text?) { true }
@@ -48,6 +53,20 @@ RSpec.describe Gitlab::Diff::PositionTracer do
subject.trace(position)
end
end
+
+ context 'position on file' do
+ let(:on_file?) { true }
+
+ it 'calls ImageStrategy#trace' do
+ expect(Gitlab::Diff::PositionTracer::FileStrategy)
+ .to receive(:new)
+ .with(subject)
+ .and_return(tracer)
+ expect(tracer).to receive(:trace).with(position)
+
+ subject.trace(position)
+ end
+ end
end
describe 'diffs methods' do
diff --git a/spec/requests/api/ci/secure_files_spec.rb b/spec/requests/api/ci/secure_files_spec.rb
index db12576154e..4e1afd66683 100644
--- a/spec/requests/api/ci/secure_files_spec.rb
+++ b/spec/requests/api/ci/secure_files_spec.rb
@@ -56,6 +56,26 @@ RSpec.describe API::Ci::SecureFiles, feature_category: :mobile_devops do
end
end
+ context 'when the feature is disabled at the instance level' do
+ before do
+ stub_config(ci_secure_files: { enabled: false })
+ end
+
+ it 'returns a 403 when attempting to upload a file' do
+ expect do
+ post api("/projects/#{project.id}/secure_files", maintainer), params: file_params
+ end.not_to change { project.secure_files.count }
+
+ expect(response).to have_gitlab_http_status(:forbidden)
+ end
+
+ it 'returns a 403 when downloading a file' do
+ get api("/projects/#{project.id}/secure_files", developer)
+
+ expect(response).to have_gitlab_http_status(:forbidden)
+ end
+ end
+
context 'when the flag is disabled' do
it 'returns a 201 when uploading a file when the ci_secure_files_read_only feature flag is disabled' do
expect do
diff --git a/spec/support/shared_examples/lib/gitlab/position_formatters_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/position_formatters_shared_examples.rb
index c9300aff3e6..1e03ddac42e 100644
--- a/spec/support/shared_examples/lib/gitlab/position_formatters_shared_examples.rb
+++ b/spec/support/shared_examples/lib/gitlab/position_formatters_shared_examples.rb
@@ -2,10 +2,9 @@
RSpec.shared_examples "position formatter" do
let(:formatter) { described_class.new(attrs) }
+ let(:key) { [123, 456, 789, Digest::SHA1.hexdigest(formatter.old_path), Digest::SHA1.hexdigest(formatter.new_path), 1, 2] }
describe '#key' do
- let(:key) { [123, 456, 789, Digest::SHA1.hexdigest(formatter.old_path), Digest::SHA1.hexdigest(formatter.new_path), 1, 2] }
-
subject { formatter.key }
it { is_expected.to eq(key) }