diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2022-12-22 12:07:22 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2022-12-22 12:07:22 +0300 |
commit | 58e69d174512e267079ebb6afc60dd097070bf35 (patch) | |
tree | d7207e894f4813a57db661de9620b9f7728d1afb | |
parent | f8edcff7e9aff93f8ac605c19e542204b0ed9ba2 (diff) |
Add latest changes from gitlab-org/gitlab@master
42 files changed, 619 insertions, 69 deletions
diff --git a/app/assets/javascripts/diff.js b/app/assets/javascripts/diff.js index 23eb470503e..65816495432 100644 --- a/app/assets/javascripts/diff.js +++ b/app/assets/javascripts/diff.js @@ -12,9 +12,13 @@ const UNFOLD_COUNT = 20; let isBound = false; export default class Diff { - constructor() { + constructor({ mergeRequestEventHub } = {}) { const $diffFile = $('.files .diff-file'); + if (mergeRequestEventHub) { + this.mrHub = mergeRequestEventHub; + } + $diffFile.each((index, file) => { if (!$.data(file, 'singleFileDiff')) { $.data(file, 'singleFileDiff', new SingleFileDiff(file)); @@ -34,7 +38,8 @@ export default class Diff { $(document) .on('click', '.js-unfold', this.handleClickUnfold.bind(this)) .on('click', '.diff-line-num a', this.handleClickLineNum.bind(this)) - .on('mousedown', 'td.line_content.parallel', this.handleParallelLineDown.bind(this)); + .on('mousedown', 'td.line_content.parallel', this.handleParallelLineDown.bind(this)) + .on('click', '.inline-parallel-buttons a', ($e) => this.viewTypeSwitch($e)); isBound = true; } @@ -135,6 +140,20 @@ export default class Diff { diffViewType() { return $('.inline-parallel-buttons a.active').data('viewType'); } + viewTypeSwitch(event) { + const click = event.originalEvent; + const diffSource = new URL(click.target.getAttribute('href'), document.location.href); + + if (this.mrHub) { + click.preventDefault(); + click.stopPropagation(); + + diffSource.pathname = `${diffSource.pathname}.json`; + + this.mrHub.$emit('diff:switch-view-type', { source: diffSource.toString() }); + } + } + // eslint-disable-next-line class-methods-use-this lineNumbers(line) { const children = line.find('.diff-line-num').toArray(); diff --git a/app/assets/javascripts/init_diff_stats_dropdown.js b/app/assets/javascripts/init_diff_stats_dropdown.js index 27df761a103..8413fe92f89 100644 --- a/app/assets/javascripts/init_diff_stats_dropdown.js +++ b/app/assets/javascripts/init_diff_stats_dropdown.js @@ -4,7 +4,7 @@ import { stickyMonitor } from './lib/utils/sticky'; export const initDiffStatsDropdown = (stickyTop) => { if (stickyTop) { - stickyMonitor(document.querySelector('.js-diff-files-changed'), stickyTop); + stickyMonitor(document.querySelector('.js-diff-files-changed'), stickyTop, false); } const el = document.querySelector('.js-diff-stats-dropdown'); diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 5a1410ceeba..46ee8fecfc5 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -1,4 +1,4 @@ -/* eslint-disable no-new, class-methods-use-this */ +/* eslint-disable class-methods-use-this */ import $ from 'jquery'; import Vue from 'vue'; import { createAlert } from '~/flash'; @@ -134,8 +134,8 @@ function destroyPipelines(app) { return null; } -function loadDiffs({ url, sticky }) { - return axios.get(`${url}.json${location.search}`).then(({ data }) => { +function loadDiffs({ url, sticky, tabs }) { + return axios.get(url).then(({ data }) => { const $container = $('#diffs'); $container.html(data.html); initDiffStatsDropdown(sticky); @@ -143,7 +143,9 @@ function loadDiffs({ url, sticky }) { localTimeAgo(document.querySelectorAll('#diffs .js-timeago')); syntaxHighlight($('#diffs .js-syntax-highlight')); - new Diff(); + tabs.createDiff(); + tabs.setHubToDiff(); + scrollToContainer('#diffs'); $('.diff-file').each((i, el) => { @@ -204,6 +206,7 @@ export default class MergeRequestTabs { this.currentTab = null; this.diffsLoaded = false; + this.diffsClass = null; this.commitsLoaded = false; this.fixedLayoutPref = null; this.eventHub = createEventHub(); @@ -211,6 +214,7 @@ export default class MergeRequestTabs { this.setUrl = setUrl !== undefined ? setUrl : true; this.setCurrentAction = this.setCurrentAction.bind(this); + this.switchViewType = this.switchViewType.bind(this); this.tabShown = this.tabShown.bind(this); this.clickTab = this.clickTab.bind(this); @@ -230,11 +234,13 @@ export default class MergeRequestTabs { this.tabShown(action, location.href); this.eventHub.$emit('MergeRequestTabChange', action); }); + this.eventHub.$on('diff:switch-view-type', this.switchViewType); } // Used in tests unbindEvents() { $('.merge-request-tabs a[data-toggle="tabvue"]').off('click', this.clickTab); + this.eventHub.$off('diff:switch-view-type', this.switchViewType); } storeScroll() { @@ -341,7 +347,7 @@ export default class MergeRequestTabs { in practice, this only occurs when comparing commits in the new merge request form page. */ - this.loadDiff(href); + this.loadDiff({ endpoint: href, strip: true }); } // this.hideSidebar(); this.expandViewContainer(); @@ -503,17 +509,20 @@ export default class MergeRequestTabs { } // load the diff tab content from the backend - loadDiff(source) { + loadDiff({ endpoint, strip = true }) { if (this.diffsLoaded) { document.dispatchEvent(new CustomEvent('scroll')); return; } + // We extract pathname for the current Changes tab anchor href + // some pages like MergeRequestsController#new has query parameters on that anchor + const diffUrl = strip ? `${parseUrlPathname(endpoint)}.json${location.search}` : endpoint; + loadDiffs({ - // We extract pathname for the current Changes tab anchor href - // some pages like MergeRequestsController#new has query parameters on that anchor - url: parseUrlPathname(source), + url: diffUrl, sticky: computeTopOffset(this.mergeRequestTabs), + tabs: this, }) .then(() => { if (this.isDiffAction(this.currentAction)) { @@ -528,6 +537,21 @@ export default class MergeRequestTabs { }); }); } + switchViewType({ source }) { + this.diffsLoaded = false; + + this.loadDiff({ endpoint: source, strip: false }); + } + createDiff() { + if (!this.diffsClass) { + this.diffsClass = new Diff({ mergeRequestEventHub: this.eventHub }); + } + } + setHubToDiff() { + if (this.diffsClass) { + this.diffsClass.mrHub = this.eventHub; + } + } diffViewType() { return $('.js-diff-view-buttons button.active').data('viewType'); diff --git a/app/assets/javascripts/pages/projects/merge_requests/init_merge_request.js b/app/assets/javascripts/pages/projects/merge_requests/init_merge_request.js index a4e3ddfc506..d4734b8842d 100644 --- a/app/assets/javascripts/pages/projects/merge_requests/init_merge_request.js +++ b/app/assets/javascripts/pages/projects/merge_requests/init_merge_request.js @@ -4,14 +4,12 @@ import $ from 'jquery'; import IssuableForm from 'ee_else_ce/issuable/issuable_form'; import IssuableLabelSelector from '~/issuable/issuable_label_selector'; import ShortcutsNavigation from '~/behaviors/shortcuts/shortcuts_navigation'; -import Diff from '~/diff'; import GLForm from '~/gl_form'; import LabelsSelect from '~/labels/labels_select'; import IssuableTemplateSelectors from '~/issuable/issuable_template_selectors'; import { mountMilestoneDropdown } from '~/sidebar/mount_sidebar'; export default () => { - new Diff(); new ShortcutsNavigation(); new GLForm($('.merge-request-form')); new IssuableForm($('.merge-request-form')); diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index 53c358f4eba..0dca5b18a24 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -14,6 +14,8 @@ module Ci include EachBatch include Gitlab::Utils::StrongMemoize + enum accessibility: { public: 0, private: 1 }, _suffix: true + NON_ERASABLE_FILE_TYPES = %w[trace].freeze REPORT_FILE_TYPES = { @@ -346,6 +348,12 @@ module Ci end end + def public_access? + return true unless Feature.enabled?(:non_public_artifacts, type: :development) + + public_accessibility? + end + private def store_file_in_transaction! diff --git a/app/policies/ci/build_policy.rb b/app/policies/ci/build_policy.rb index 5ef926ef2e3..ca0b51e1385 100644 --- a/app/policies/ci/build_policy.rb +++ b/app/policies/ci/build_policy.rb @@ -59,7 +59,7 @@ module Ci @subject.debug_mode? end - condition(:project_read_build, scope: :subject) do + condition(:can_read_project_build, scope: :subject) do can?(:read_build, @subject.project) end @@ -71,7 +71,7 @@ module Ci can?(:developer_access, @subject.project) end - rule { project_read_build }.enable :read_build_trace + rule { can_read_project_build }.enable :read_build_trace rule { debug_mode & ~project_update_build }.prevent :read_build_trace # Authorizing the user to access to protected entities. @@ -114,7 +114,7 @@ module Ci prevent :create_build_service_proxy end - rule { project_read_build }.enable :read_job_artifacts + rule { can_read_project_build }.enable :read_job_artifacts rule { ~artifacts_public & ~project_developer }.prevent :read_job_artifacts end end diff --git a/app/policies/ci/job_artifact_policy.rb b/app/policies/ci/job_artifact_policy.rb index e25c7311565..61c935af8ba 100644 --- a/app/policies/ci/job_artifact_policy.rb +++ b/app/policies/ci/job_artifact_policy.rb @@ -3,5 +3,20 @@ module Ci class JobArtifactPolicy < BasePolicy delegate { @subject.job.project } + + condition(:public_access, scope: :subject) do + @subject.public_access? + end + + condition(:can_read_project_build, scope: :subject) do + can?(:read_build, @subject.job.project) + end + + condition(:has_access_to_project) do + can?(:developer_access, @subject.job.project) + end + + rule { can_read_project_build }.enable :read_job_artifacts + rule { ~public_access & ~has_access_to_project }.prevent :read_job_artifacts end end diff --git a/app/serializers/ci/downloadable_artifact_entity.rb b/app/serializers/ci/downloadable_artifact_entity.rb index 1f3885f0715..2e8aafcee43 100644 --- a/app/serializers/ci/downloadable_artifact_entity.rb +++ b/app/serializers/ci/downloadable_artifact_entity.rb @@ -8,7 +8,7 @@ module Ci artifacts = pipeline.downloadable_artifacts if Feature.enabled?(:non_public_artifacts) - artifacts = artifacts.select { |artifact| can?(request.current_user, :read_job_artifacts, artifact.job) } + artifacts = artifacts.select { |artifact| can?(request.current_user, :read_job_artifacts, artifact) } end BuildArtifactEntity.represent(artifacts, options.merge(project: pipeline.project)) diff --git a/app/serializers/merge_requests/pipeline_entity.rb b/app/serializers/merge_requests/pipeline_entity.rb index 76e75a8ca6d..445914fe01c 100644 --- a/app/serializers/merge_requests/pipeline_entity.rb +++ b/app/serializers/merge_requests/pipeline_entity.rb @@ -30,7 +30,7 @@ class MergeRequests::PipelineEntity < Grape::Entity rel = pipeline.downloadable_artifacts if Feature.enabled?(:non_public_artifacts, type: :development) - rel = rel.select { |artifact| can?(request.current_user, :read_job_artifacts, artifact.job) } + rel = rel.select { |artifact| can?(request.current_user, :read_job_artifacts, artifact) } end BuildArtifactEntity.represent(rel, options.merge(project: pipeline.project)) diff --git a/app/services/ci/job_artifacts/create_service.rb b/app/services/ci/job_artifacts/create_service.rb index ee9982cf3ab..6e2ba76682f 100644 --- a/app/services/ci/job_artifacts/create_service.rb +++ b/app/services/ci/job_artifacts/create_service.rb @@ -92,7 +92,8 @@ module Ci file: artifacts_file, file_type: params[:artifact_type], file_format: params[:artifact_format], - file_sha256: artifacts_file.sha256 + file_sha256: artifacts_file.sha256, + accessibility: accessibility(params) ) ) @@ -102,7 +103,8 @@ module Ci file: metadata_file, file_type: :metadata, file_format: :gzip, - file_sha256: metadata_file.sha256 + file_sha256: metadata_file.sha256, + accessibility: accessibility(params) ) ) end @@ -110,6 +112,10 @@ module Ci [artifact, artifact_metadata] end + def accessibility(params) + params[:accessibility] || 'public' + end + def parse_artifact(artifact) case artifact.file_type when 'dotenv' then parse_dotenv_artifact(artifact) diff --git a/db/migrate/20221010191136_add_access_level_to_ci_job_artifacts.rb b/db/migrate/20221010191136_add_access_level_to_ci_job_artifacts.rb new file mode 100644 index 00000000000..d69965ed8ec --- /dev/null +++ b/db/migrate/20221010191136_add_access_level_to_ci_job_artifacts.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddAccessLevelToCiJobArtifacts < Gitlab::Database::Migration[2.1] + enable_lock_retries! + + def change + add_column :ci_job_artifacts, :accessibility, :integer, default: 0, limit: 2, null: false + end +end diff --git a/db/post_migrate/20221221110733_remove_temp_index_for_project_statistics_upload_size_migration.rb b/db/post_migrate/20221221110733_remove_temp_index_for_project_statistics_upload_size_migration.rb new file mode 100644 index 00000000000..1df6ad274f9 --- /dev/null +++ b/db/post_migrate/20221221110733_remove_temp_index_for_project_statistics_upload_size_migration.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class RemoveTempIndexForProjectStatisticsUploadSizeMigration < Gitlab::Database::Migration[2.1] + INDEX_NAME = 'tmp_index_project_statistics_uploads_size' + TABLE_NAME = 'project_statistics' + disable_ddl_transaction! + + def up + remove_concurrent_index_by_name :project_statistics, INDEX_NAME + end + + def down + add_concurrent_index :project_statistics, [:project_id], + name: INDEX_NAME, + where: "uploads_size <> 0" + end +end diff --git a/db/schema_migrations/20221010191136 b/db/schema_migrations/20221010191136 new file mode 100644 index 00000000000..00128d6ce52 --- /dev/null +++ b/db/schema_migrations/20221010191136 @@ -0,0 +1 @@ +031607378457cac9f9477e751f2ebe15173a91fec98daa4e64b1f278dce5d931
\ No newline at end of file diff --git a/db/schema_migrations/20221221110733 b/db/schema_migrations/20221221110733 new file mode 100644 index 00000000000..6900431db9b --- /dev/null +++ b/db/schema_migrations/20221221110733 @@ -0,0 +1 @@ +db73b1dca175b51bfb9a5fd20806f746cc9d80d37d1eed7c2958a6dfd1445796
\ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index c9b605c726f..bfa95ac0fa1 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -12946,6 +12946,7 @@ CREATE TABLE ci_job_artifacts ( locked smallint DEFAULT 2, original_filename text, partition_id bigint DEFAULT 100 NOT NULL, + accessibility smallint DEFAULT 0 NOT NULL, CONSTRAINT check_27f0f6dbab CHECK ((file_store IS NOT NULL)), CONSTRAINT check_85573000db CHECK ((char_length(original_filename) <= 512)) ); @@ -31703,8 +31704,6 @@ CREATE INDEX tmp_index_on_vulnerabilities_non_dismissed ON vulnerabilities USING CREATE INDEX tmp_index_project_statistics_cont_registry_size ON project_statistics USING btree (project_id) WHERE (container_registry_size = 0); -CREATE INDEX tmp_index_project_statistics_uploads_size ON project_statistics USING btree (project_id) WHERE (uploads_size <> 0); - CREATE UNIQUE INDEX uniq_pkgs_deb_grp_architectures_on_distribution_id_and_name ON packages_debian_group_architectures USING btree (distribution_id, name); CREATE UNIQUE INDEX uniq_pkgs_deb_grp_components_on_distribution_id_and_name ON packages_debian_group_components USING btree (distribution_id, name); diff --git a/doc/administration/package_information/supported_os.md b/doc/administration/package_information/supported_os.md index fdbb17f3c71..5dee7790e42 100644 --- a/doc/administration/package_information/supported_os.md +++ b/doc/administration/package_information/supported_os.md @@ -40,6 +40,9 @@ CentOS 8 was EOL on December 31, 2021. In GitLab 14.5 and later, We officially support all distributions that are binary compatible with Red Hat Enterprise Linux. This gives users a path forward for their CentOS 8 builds at its end of life. +NOTE: +The [CentOS major version and a minor version](https://en.wikipedia.org/wiki/CentOS#CentOS_releases) up to CentOS8 ([when CentOS Stream](https://en.wikipedia.org/wiki/CentOS#CentOS_Stream) was released) correspond to the set of major version and update versions of RHEL. + ## Update GitLab package sources after upgrading the OS After upgrading the Operating System (OS) as per its own documentation, diff --git a/doc/ci/yaml/index.md b/doc/ci/yaml/index.md index 6e6bb68894e..873e8c93fbd 100644 --- a/doc/ci/yaml/index.md +++ b/doc/ci/yaml/index.md @@ -1003,7 +1003,7 @@ rspec: - Combining reports in parent pipelines using [artifacts from child pipelines](#needspipelinejob) is not supported. Track progress on adding support in [this issue](https://gitlab.com/gitlab-org/gitlab/-/issues/215725). -- To be able to browse the report output files, include the [`artifacts:paths`](#artifactspaths) keyword. This will upload and store the artifact twice. +- To be able to browse the report output files, include the [`artifacts:paths`](#artifactspaths) keyword. This uploads and stores the artifact twice. - The test reports are collected regardless of the job results (success or failure). You can use [`artifacts:expire_in`](#artifactsexpire_in) to set up an expiration date for artifacts reports. diff --git a/doc/ci/yaml/yaml_optimization.md b/doc/ci/yaml/yaml_optimization.md index 5344a999b95..2d875c0e250 100644 --- a/doc/ci/yaml/yaml_optimization.md +++ b/doc/ci/yaml/yaml_optimization.md @@ -13,7 +13,7 @@ files by using: - YAML-specific features like [anchors (`&`)](#anchors), aliases (`*`), and map merging (`<<`). Read more about the various [YAML features](https://learnxinyminutes.com/docs/yaml/). - The [`extends` keyword](#use-extends-to-reuse-configuration-sections), - which is more flexible and readable. We recommend you use `extends` where possible. + which is more flexible and readable. You should use `extends` where possible. ## Anchors diff --git a/doc/user/project/remote_development/index.md b/doc/user/project/remote_development/index.md index 71cc6f19c6b..1abcca547db 100644 --- a/doc/user/project/remote_development/index.md +++ b/doc/user/project/remote_development/index.md @@ -157,5 +157,4 @@ Alternatively, you can pass the parameters from a URL and connect directly to th ## Related topics -- [Web IDE](../web_ide/index.md) - [Web IDE Beta](../web_ide_beta/index.md) diff --git a/doc/user/project/web_ide_beta/index.md b/doc/user/project/web_ide_beta/index.md index 85913874be7..4f84110a038 100644 --- a/doc/user/project/web_ide_beta/index.md +++ b/doc/user/project/web_ide_beta/index.md @@ -13,10 +13,10 @@ On self-managed GitLab, by default this feature is not available. To make it ava As announced in [this blog post](https://about.gitlab.com/blog/2022/05/23/the-future-of-the-gitlab-web-ide/), the current implementation of the [Web IDE](../web_ide/index.md) is being replaced -with an implementation inspired by Visual Studio Code. This effort is still under +with an implementation powered by Visual Studio Code. This effort is still under development. For updates, see [this epic](https://gitlab.com/groups/gitlab-org/-/epics/7683). -To connect a remote machine to the Web IDE Beta, see [Remote Development](../remote_development/index.md). +To pair the Web IDE Beta with a Remote Development environment, see [Remote Development](../remote_development/index.md). ## Enable the Web IDE Beta diff --git a/lib/api/ci/runner.rb b/lib/api/ci/runner.rb index b073eb49bf1..6b4394114df 100644 --- a/lib/api/ci/runner.rb +++ b/lib/api/ci/runner.rb @@ -312,6 +312,7 @@ module API optional :artifact_format, type: String, desc: %q(The format of artifact), default: 'zip', values: ::Ci::JobArtifact.file_formats.keys optional :metadata, type: ::API::Validations::Types::WorkhorseFile, desc: %(The artifact metadata to store (generated by Multipart middleware)), documentation: { type: 'file' } + optional :accessibility, type: String, desc: %q(Specify accessibility level of artifact private/public) end post '/:id/artifacts', feature_category: :build_artifacts, urgency: :low do not_allowed! unless Gitlab.config.artifacts.enabled diff --git a/lib/object_storage/direct_upload.rb b/lib/object_storage/direct_upload.rb index 9449e51b053..7d2f825e119 100644 --- a/lib/object_storage/direct_upload.rb +++ b/lib/object_storage/direct_upload.rb @@ -44,6 +44,7 @@ module ObjectStorage GetURL: get_url, StoreURL: store_url, DeleteURL: delete_url, + SkipDelete: false, MultipartUpload: multipart_upload_hash, CustomPutHeaders: true, PutHeaders: upload_options diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 177789089cb..af447da9876 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -10059,6 +10059,9 @@ msgstr "" msgid "Committed by" msgstr "" +msgid "CommonJS module" +msgstr "" + msgid "Community forum" msgstr "" @@ -14847,6 +14850,9 @@ msgstr "" msgid "E-mail:" msgstr "" +msgid "ESM module" +msgstr "" + msgid "Each project can also have an issue tracker and a wiki." msgstr "" @@ -20092,6 +20098,9 @@ msgstr "" msgid "HAR file path or URL" msgstr "" +msgid "HTML script tag" +msgstr "" + msgid "HTTP Archive (HAR)" msgstr "" @@ -31608,15 +31617,54 @@ msgstr "" msgid "Proceed" msgstr "" +msgid "Product Analytics|Add the NPM package to your package.json using your preferred package manager:" +msgstr "" + +msgid "Product Analytics|Add the script to the page and assign the client SDK to window:" +msgstr "" + msgid "Product Analytics|Analyze your product with Product Analytics" msgstr "" +msgid "Product Analytics|For the product analytics dashboard to start showing you some data, you need to add the analytics tracking code to your project." +msgstr "" + +msgid "Product Analytics|Identifies the sender of tracking events" +msgstr "" + +msgid "Product Analytics|Import the new package into your JS code:" +msgstr "" + +msgid "Product Analytics|Instrument your application" +msgstr "" + +msgid "Product Analytics|SDK App ID" +msgstr "" + +msgid "Product Analytics|SDK Host" +msgstr "" + msgid "Product Analytics|Set up Product Analytics to track how your product is performing. Combine it with your GitLab data to better understand where you can improve your product and development processes." msgstr "" msgid "Product Analytics|Set up product analytics..." msgstr "" +msgid "Product Analytics|Steps to add product analytics as a CommonJS module" +msgstr "" + +msgid "Product Analytics|Steps to add product analytics as a HTML script tag" +msgstr "" + +msgid "Product Analytics|Steps to add product analytics as an ESM module" +msgstr "" + +msgid "Product Analytics|The host to send all tracking events to" +msgstr "" + +msgid "Product Analytics|To instrument your application, select one of the options below. After an option has been instrumented and data is being collected, this page will progress to the next step." +msgstr "" + msgid "Product analytics" msgstr "" diff --git a/qa/qa/specs/features/api/1_manage/project_access_token_spec.rb b/qa/qa/specs/features/api/1_manage/project_access_token_spec.rb index 34f0c381be1..d693bbd43ff 100644 --- a/qa/qa/specs/features/api/1_manage/project_access_token_spec.rb +++ b/qa/qa/specs/features/api/1_manage/project_access_token_spec.rb @@ -14,7 +14,7 @@ module QA end context 'for the same project' do - it 'can be used to create a file via the project API', :reliable, testcase: 'https://gitlab.com/gitlab-org/gitlab/-/quality/test_cases/347858' do + it 'can be used to create a file via the project API', testcase: 'https://gitlab.com/gitlab-org/gitlab/-/quality/test_cases/347858' do expect do Resource::File.fabricate_via_api! do |file| file.api_client = @user_api_client @@ -46,7 +46,7 @@ module QA @different_project = Resource::Project.fabricate! end - it 'cannot be used to create a file via the project API', :reliable, testcase: 'https://gitlab.com/gitlab-org/gitlab/-/quality/test_cases/347860' do + it 'cannot be used to create a file via the project API', testcase: 'https://gitlab.com/gitlab-org/gitlab/-/quality/test_cases/347860' do expect do Resource::File.fabricate_via_api! do |file| file.api_client = @user_api_client @@ -59,7 +59,7 @@ module QA end.to raise_error(Resource::ApiFabricator::ResourceFabricationFailedError, /403 Forbidden/) end - it 'cannot be used to commit via the API', :reliable, testcase: 'https://gitlab.com/gitlab-org/gitlab/-/quality/test_cases/347861' do + it 'cannot be used to commit via the API', testcase: 'https://gitlab.com/gitlab-org/gitlab/-/quality/test_cases/347861' do expect do Resource::Repository::Commit.fabricate_via_api! do |commit| commit.api_client = @user_api_client diff --git a/qa/qa/specs/features/browser_ui/3_create/pages/new_static_page_spec.rb b/qa/qa/specs/features/browser_ui/3_create/pages/new_static_page_spec.rb index 449bffe61e0..4dc93bdf112 100644 --- a/qa/qa/specs/features/browser_ui/3_create/pages/new_static_page_spec.rb +++ b/qa/qa/specs/features/browser_ui/3_create/pages/new_static_page_spec.rb @@ -9,7 +9,8 @@ module QA issue: 'https://gitlab.com/gitlab-org/gitlab/-/issues/383215', type: :test_environment, only: { subdomain: 'staging-ref' } - } do + }, + feature_flag: { name: 'show_pages_in_deployments_menu' } do # TODO: Convert back to :smoke once proved to be stable. Related issue: https://gitlab.com/gitlab-org/gitlab/-/issues/300906 describe 'Pages', product_group: :editor do let!(:project) do @@ -29,6 +30,9 @@ module QA end before do + # Pages Menu Experiment currently progress https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98044 + # Update spec along with Feature Flag Removal. + Runtime::Feature.disable(:show_pages_in_deployments_menu) Flow::Login.sign_in Resource::Runner.fabricate_via_api! do |runner| runner.project = project @@ -37,6 +41,10 @@ module QA pipeline.visit! end + after do + Runtime::Feature.enable(:show_pages_in_deployments_menu) + end + it 'creates a Pages website', testcase: 'https://gitlab.com/gitlab-org/gitlab/-/quality/test_cases/347669' do Page::Project::Pipeline::Show.perform do |show| diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 15a88955e05..78398fd7f20 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -421,9 +421,17 @@ FactoryBot.define do end trait :artifacts do - after(:create) do |build| - create(:ci_job_artifact, :archive, job: build, expire_at: build.artifacts_expire_at) - create(:ci_job_artifact, :metadata, job: build, expire_at: build.artifacts_expire_at) + after(:create) do |build, evaluator| + create(:ci_job_artifact, :archive, :public, job: build, expire_at: build.artifacts_expire_at) + create(:ci_job_artifact, :metadata, :public, job: build, expire_at: build.artifacts_expire_at) + build.reload + end + end + + trait :private_artifacts do + after(:create) do |build, evaluator| + create(:ci_job_artifact, :archive, :private, job: build, expire_at: build.artifacts_expire_at) + create(:ci_job_artifact, :metadata, :private, job: build, expire_at: build.artifacts_expire_at) build.reload end end diff --git a/spec/factories/ci/job_artifacts.rb b/spec/factories/ci/job_artifacts.rb index 7569e832c60..5e049e0375b 100644 --- a/spec/factories/ci/job_artifacts.rb +++ b/spec/factories/ci/job_artifacts.rb @@ -174,6 +174,14 @@ FactoryBot.define do end end + trait :private do + accessibility { 'private' } + end + + trait :public do + accessibility { 'public' } + end + trait :accessibility do file_type { :accessibility } file_format { :raw } diff --git a/spec/frontend/diff_spec.js b/spec/frontend/diff_spec.js new file mode 100644 index 00000000000..759ae32ac51 --- /dev/null +++ b/spec/frontend/diff_spec.js @@ -0,0 +1,72 @@ +import createEventHub from '~/helpers/event_hub_factory'; + +import Diff from '~/diff'; + +describe('Diff', () => { + describe('diff <-> tabs interactions', () => { + let hub; + + beforeEach(() => { + hub = createEventHub(); + }); + + describe('constructor', () => { + it("takes in the `mergeRequestEventHub` when it's provided", () => { + const diff = new Diff({ mergeRequestEventHub: hub }); + + expect(diff.mrHub).toBe(hub); + }); + + it('does not fatal if no event hub is provided', () => { + expect(() => { + new Diff(); /* eslint-disable-line no-new */ + }).not.toThrow(); + }); + + it("doesn't set the mrHub property if none is provided by the construction arguments", () => { + const diff = new Diff(); + + expect(diff.mrHub).toBe(undefined); + }); + }); + + describe('viewTypeSwitch', () => { + const clickPath = '/path/somewhere?params=exist'; + const jsonPath = 'http://test.host/path/somewhere.json?params=exist'; + const simulatejQueryClick = { + originalEvent: { + target: { + getAttribute() { + return clickPath; + }, + }, + preventDefault: jest.fn(), + stopPropagation: jest.fn(), + }, + }; + + it('emits the correct switch view event when called and there is an `mrHub`', async () => { + const diff = new Diff({ mergeRequestEventHub: hub }); + const hubEmit = new Promise((resolve) => { + hub.$on('diff:switch-view-type', resolve); + }); + + diff.viewTypeSwitch(simulatejQueryClick); + const { source } = await hubEmit; + + expect(simulatejQueryClick.originalEvent.preventDefault).toHaveBeenCalled(); + expect(simulatejQueryClick.originalEvent.stopPropagation).toHaveBeenCalled(); + expect(source).toBe(jsonPath); + }); + + it('is effectively a noop when there is no `mrHub`', () => { + const diff = new Diff(); + + expect(diff.mrHub).toBe(undefined); + expect(() => { + diff.viewTypeSwitch(simulatejQueryClick); + }).not.toThrow(); + }); + }); + }); +}); diff --git a/spec/frontend/merge_request_tabs_spec.js b/spec/frontend/merge_request_tabs_spec.js index 69ff5e47689..6d434d7e654 100644 --- a/spec/frontend/merge_request_tabs_spec.js +++ b/spec/frontend/merge_request_tabs_spec.js @@ -5,6 +5,7 @@ import initMrPage from 'helpers/init_vue_mr_page_helper'; import { stubPerformanceWebAPI } from 'helpers/performance'; import axios from '~/lib/utils/axios_utils'; import MergeRequestTabs from '~/merge_request_tabs'; +import Diff from '~/diff'; import '~/lib/utils/common_utils'; import '~/lib/utils/url_utility'; @@ -389,4 +390,73 @@ describe('MergeRequestTabs', () => { }); }); }); + + describe('tabs <-> diff interactions', () => { + beforeEach(() => { + jest.spyOn(testContext.class, 'loadDiff').mockImplementation(() => {}); + }); + + describe('switchViewType', () => { + it('marks the class as having not loaded diffs already', () => { + testContext.class.diffsLoaded = true; + + testContext.class.switchViewType({}); + + expect(testContext.class.diffsLoaded).toBe(false); + }); + + it('reloads the diffs', () => { + testContext.class.switchViewType({ source: 'a new url' }); + + expect(testContext.class.loadDiff).toHaveBeenCalledWith({ + endpoint: 'a new url', + strip: false, + }); + }); + }); + + describe('createDiff', () => { + it("creates a Diff if there isn't one", () => { + expect(testContext.class.diffsClass).toBe(null); + + testContext.class.createDiff(); + + expect(testContext.class.diffsClass).toBeInstanceOf(Diff); + }); + + it("doesn't create a Diff if one already exists", () => { + testContext.class.diffsClass = 'truthy'; + + testContext.class.createDiff(); + + expect(testContext.class.diffsClass).toBe('truthy'); + }); + + it('sets the available MR Tabs event hub to the new Diff', () => { + expect(testContext.class.diffsClass).toBe(null); + + testContext.class.createDiff(); + + expect(testContext.class.diffsClass.mrHub).toBe(testContext.class.eventHub); + }); + }); + + describe('setHubToDiff', () => { + it('sets the MR Tabs event hub to the child Diff', () => { + testContext.class.diffsClass = {}; + + testContext.class.setHubToDiff(); + + expect(testContext.class.diffsClass.mrHub).toBe(testContext.class.eventHub); + }); + + it('does not fatal if theres no child Diff', () => { + testContext.class.diffsClass = null; + + expect(() => { + testContext.class.setHubToDiff(); + }).not.toThrow(); + }); + }); + }); }); diff --git a/spec/lib/object_storage/direct_upload_spec.rb b/spec/lib/object_storage/direct_upload_spec.rb index c2201fb60ac..569e6a3a7c6 100644 --- a/spec/lib/object_storage/direct_upload_spec.rb +++ b/spec/lib/object_storage/direct_upload_spec.rb @@ -234,6 +234,7 @@ RSpec.describe ObjectStorage::DirectUpload do expect(subject[:GetURL]).to start_with(storage_url) expect(subject[:StoreURL]).to start_with(storage_url) expect(subject[:DeleteURL]).to start_with(storage_url) + expect(subject[:SkipDelete]).to eq(false) expect(subject[:CustomPutHeaders]).to be_truthy expect(subject[:PutHeaders]).to eq({}) end diff --git a/spec/migrations/20221221110733_remove_temp_index_for_project_statistics_upload_size_migration_spec.rb b/spec/migrations/20221221110733_remove_temp_index_for_project_statistics_upload_size_migration_spec.rb new file mode 100644 index 00000000000..6f9cfe4764a --- /dev/null +++ b/spec/migrations/20221221110733_remove_temp_index_for_project_statistics_upload_size_migration_spec.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_migration! + +RSpec.describe RemoveTempIndexForProjectStatisticsUploadSizeMigration, +feature_category: :subscription_cost_management do + let(:table_name) { 'project_statistics' } + let(:index_name) { described_class::INDEX_NAME } + + it 'correctly migrates up and down' do + reversible_migration do |migration| + migration.before -> { + expect(subject.index_exists_by_name?(table_name, index_name)).to be_truthy + } + + migration.after -> { + expect(subject.index_exists_by_name?(table_name, index_name)).to be_falsy + } + end + end +end diff --git a/spec/models/ci/job_artifact_spec.rb b/spec/models/ci/job_artifact_spec.rb index 5477eb9fb5d..509f358dad3 100644 --- a/spec/models/ci/job_artifact_spec.rb +++ b/spec/models/ci/job_artifact_spec.rb @@ -134,6 +134,38 @@ RSpec.describe Ci::JobArtifact do end end + describe 'artifacts_public?' do + subject { artifact.public_access? } + + context 'when job artifact created by default' do + let!(:artifact) { create(:ci_job_artifact) } + + it { is_expected.to be_truthy } + end + + context 'when job artifact created as public' do + let!(:artifact) { create(:ci_job_artifact, :public) } + + it { is_expected.to be_truthy } + end + + context 'when job artifact created as private' do + let!(:artifact) { build(:ci_job_artifact, :private) } + + it { is_expected.to be_falsey } + + context 'and the non_public_artifacts feature flag is disabled' do + let!(:artifact) { build(:ci_job_artifact, :private) } + + before do + stub_feature_flags(non_public_artifacts: false) + end + + it { is_expected.to be_truthy } + end + end + end + describe '.file_types_for_report' do it 'returns the report file types for the report type' do expect(described_class.file_types_for_report(:test)).to match_array(%w[junit]) diff --git a/spec/requests/api/ci/runner/jobs_artifacts_spec.rb b/spec/requests/api/ci/runner/jobs_artifacts_spec.rb index 1c119079c50..3d3d699542b 100644 --- a/spec/requests/api/ci/runner/jobs_artifacts_spec.rb +++ b/spec/requests/api/ci/runner/jobs_artifacts_spec.rb @@ -575,6 +575,45 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state, feature_catego end end + context 'when access level is private' do + subject(:request) { upload_artifacts(file_upload, headers_with_token, params) } + + let(:params) { { artifact_type: :archive, artifact_format: :zip, accessibility: 'private' } } + + it 'sets job artifact access level to private' do + subject + + expect(response).to have_gitlab_http_status(:created) + expect(job.reload.job_artifacts_archive).to be_private_accessibility + end + end + + context 'when access level is public' do + subject(:request) { upload_artifacts(file_upload, headers_with_token, params) } + + let(:params) { { artifact_type: :archive, artifact_format: :zip, accessibility: 'public' } } + + it 'sets job artifact access level to public' do + subject + + expect(response).to have_gitlab_http_status(:created) + expect(job.reload.job_artifacts_archive).to be_public_accessibility + end + end + + context 'when access level is unknown' do + subject(:request) { upload_artifacts(file_upload, headers_with_token, params) } + + let(:params) { { artifact_type: :archive, artifact_format: :zip } } + + it 'sets job artifact access level to public' do + subject + + expect(response).to have_gitlab_http_status(:created) + expect(job.reload.job_artifacts_archive).to be_public_accessibility + end + end + context 'when artifact_type is archive' do context 'when artifact_format is zip' do subject(:request) { upload_artifacts(file_upload, headers_with_token, params) } diff --git a/spec/serializers/ci/downloadable_artifact_entity_spec.rb b/spec/serializers/ci/downloadable_artifact_entity_spec.rb index 34a271e7422..3142b03581d 100644 --- a/spec/serializers/ci/downloadable_artifact_entity_spec.rb +++ b/spec/serializers/ci/downloadable_artifact_entity_spec.rb @@ -17,7 +17,10 @@ RSpec.describe Ci::DownloadableArtifactEntity do end context 'when user cannot read job artifact' do - let!(:build) { create(:ci_build, :success, :artifacts, :non_public_artifacts, pipeline: pipeline) } + let!(:build) do + create(:ci_build, :success, :private_artifacts, + pipeline: pipeline) + end it 'returns only artifacts readable by user', :aggregate_failures do expect(subject[:artifacts].size).to eq(1) diff --git a/spec/services/ci/job_artifacts/create_service_spec.rb b/spec/services/ci/job_artifacts/create_service_spec.rb index 5df590a1b78..711002e28af 100644 --- a/spec/services/ci/job_artifacts/create_service_spec.rb +++ b/spec/services/ci/job_artifacts/create_service_spec.rb @@ -61,6 +61,49 @@ RSpec.describe Ci::JobArtifacts::CreateService do expect(new_artifact.locked).to eq(job.pipeline.locked) end + it 'sets accessibility level by default to public' do + expect { subject }.to change { Ci::JobArtifact.count }.by(1) + + new_artifact = job.job_artifacts.last + expect(new_artifact).to be_public_accessibility + end + + context 'when accessibility level passed as private' do + before do + params.merge!('accessibility' => 'private') + end + + it 'sets accessibility level to private' do + expect { subject }.to change { Ci::JobArtifact.count }.by(1) + + new_artifact = job.job_artifacts.last + expect(new_artifact).to be_private_accessibility + end + end + + context 'when accessibility passed as public' do + before do + params.merge!('accessibility' => 'public') + end + + it 'sets accessibility to public level' do + expect { subject }.to change { Ci::JobArtifact.count }.by(1) + + new_artifact = job.job_artifacts.last + expect(new_artifact).to be_public_accessibility + end + end + + context 'when accessibility passed as invalid value' do + before do + params.merge!('accessibility' => 'invalid_value') + end + + it 'fails with argument error' do + expect { subject }.to raise_error(ArgumentError) + end + end + context 'when metadata file is also uploaded' do let(:metadata_file) do file_to_upload('spec/fixtures/ci_build_artifacts_metadata.gz', sha256: artifacts_sha256) @@ -82,6 +125,39 @@ RSpec.describe Ci::JobArtifacts::CreateService do expect(new_artifact.locked).to eq(job.pipeline.locked) end + it 'sets accessibility by default to public' do + expect { subject }.to change { Ci::JobArtifact.count }.by(2) + + new_artifact = job.job_artifacts.last + expect(new_artifact).to be_public_accessibility + end + + context 'when accessibility level passed as private' do + before do + params.merge!('accessibility' => 'private') + end + + it 'sets accessibility to private level' do + expect { subject }.to change { Ci::JobArtifact.count }.by(2) + + new_artifact = job.job_artifacts.last + expect(new_artifact).to be_private_accessibility + end + end + + context 'when accessibility passed as public' do + before do + params.merge!('accessibility' => 'public') + end + + it 'sets accessibility level to public' do + expect { subject }.to change { Ci::JobArtifact.count }.by(2) + + new_artifact = job.job_artifacts.last + expect(new_artifact).to be_public_accessibility + end + end + it 'sets expiration date according to application settings' do expected_expire_at = 1.day.from_now diff --git a/workhorse/internal/api/api.go b/workhorse/internal/api/api.go index 1758bb5a6a8..92e88ae3f70 100644 --- a/workhorse/internal/api/api.go +++ b/workhorse/internal/api/api.go @@ -98,6 +98,8 @@ type RemoteObject struct { GetURL string // DeleteURL is a presigned S3 RemoveObject URL DeleteURL string + // Whether Workhorse needs to delete the temporary object or not. + SkipDelete bool // StoreURL is the temporary presigned S3 PutObject URL to which upload the first found file StoreURL string // Boolean to indicate whether to use headers included in PutHeaders diff --git a/workhorse/internal/upload/destination/destination.go b/workhorse/internal/upload/destination/destination.go index 5e145e2cb2a..0a7e2493cd6 100644 --- a/workhorse/internal/upload/destination/destination.go +++ b/workhorse/internal/upload/destination/destination.go @@ -108,6 +108,7 @@ func (fh *FileHandler) GitLabFinalizeFields(prefix string) (map[string]string, e type consumer interface { Consume(context.Context, io.Reader, time.Time) (int64, error) + ConsumeWithoutDelete(context.Context, io.Reader, time.Time) (int64, error) } // Upload persists the provided reader content to all the location specified in opts. A cleanup will be performed once ctx is Done @@ -185,7 +186,12 @@ func Upload(ctx context.Context, reader io.Reader, size int64, name string, opts reader = hlr } - fh.Size, err = uploadDestination.Consume(ctx, reader, opts.Deadline) + if opts.SkipDelete { + fh.Size, err = uploadDestination.ConsumeWithoutDelete(ctx, reader, opts.Deadline) + } else { + fh.Size, err = uploadDestination.Consume(ctx, reader, opts.Deadline) + } + if err != nil { if (err == objectstore.ErrNotEnoughParts) || (hlr != nil && hlr.n < 0) { err = ErrEntityTooLarge diff --git a/workhorse/internal/upload/destination/destination_test.go b/workhorse/internal/upload/destination/destination_test.go index b355935e347..928ffdb9f5a 100644 --- a/workhorse/internal/upload/destination/destination_test.go +++ b/workhorse/internal/upload/destination/destination_test.go @@ -134,13 +134,17 @@ func TestUpload(t *testing.T) { tmpFolder := t.TempDir() tests := []struct { - name string - local bool - remote remote + name string + local bool + remote remote + skipDelete bool }{ {name: "Local only", local: true}, {name: "Remote Single only", remote: remoteSingle}, {name: "Remote Multipart only", remote: remoteMultipart}, + {name: "Local only With SkipDelete", local: true, skipDelete: true}, + {name: "Remote Single only With SkipDelete", remote: remoteSingle, skipDelete: true}, + {name: "Remote Multipart only With SkipDelete", remote: remoteMultipart, skipDelete: true}, } for _, spec := range tests { @@ -183,6 +187,12 @@ func TestUpload(t *testing.T) { opts.LocalTempPath = tmpFolder } + opts.SkipDelete = spec.skipDelete + + if opts.SkipDelete { + expectedDeletes = 0 + } + ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -343,31 +353,48 @@ func TestUploadWithUnknownGoCloudScheme(t *testing.T) { } func TestUploadMultipartInBodyFailure(t *testing.T) { - osStub, ts := test.StartObjectStore() - defer ts.Close() - - // this is a broken path because it contains bucket name but no key - // this is the only way to get an in-body failure from our ObjectStoreStub - objectPath := "/bucket-but-no-object-key" - objectURL := ts.URL + objectPath - opts := UploadOpts{ - RemoteID: "test-file", - RemoteURL: objectURL, - PartSize: test.ObjectSize, - PresignedParts: []string{objectURL + "?partNumber=1", objectURL + "?partNumber=2"}, - PresignedCompleteMultipart: objectURL + "?Signature=CompleteSignature", - Deadline: testDeadline(), + tests := []struct { + name string + skipDelete bool + }{ + {name: "With skipDelete false", skipDelete: false}, + {name: "With skipDelete true", skipDelete: true}, } - osStub.InitiateMultipartUpload(objectPath) + for _, spec := range tests { + t.Run(spec.name, func(t *testing.T) { + osStub, ts := test.StartObjectStore() + defer ts.Close() - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() + // this is a broken path because it contains bucket name but no key + // this is the only way to get an in-body failure from our ObjectStoreStub + objectPath := "/bucket-but-no-object-key" + objectURL := ts.URL + objectPath + opts := UploadOpts{ + RemoteID: "test-file", + RemoteURL: objectURL, + PartSize: test.ObjectSize, + PresignedParts: []string{objectURL + "?partNumber=1", objectURL + "?partNumber=2"}, + PresignedCompleteMultipart: objectURL + "?Signature=CompleteSignature", + PresignedDelete: objectURL + "?Signature=AnotherSignature", + Deadline: testDeadline(), + SkipDelete: spec.skipDelete, + } - fh, err := Upload(ctx, strings.NewReader(test.ObjectContent), test.ObjectSize, "upload", &opts) - require.Nil(t, fh) - require.Error(t, err) - require.EqualError(t, err, test.MultipartUploadInternalError().Error()) + osStub.InitiateMultipartUpload(objectPath) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + fh, err := Upload(ctx, strings.NewReader(test.ObjectContent), test.ObjectSize, "upload", &opts) + require.Nil(t, fh) + require.Error(t, err) + require.EqualError(t, err, test.MultipartUploadInternalError().Error()) + + cancel() // this will trigger an async cleanup + requireObjectStoreDeletedAsync(t, 1, osStub) + }) + } } func TestUploadRemoteFileWithLimit(t *testing.T) { diff --git a/workhorse/internal/upload/destination/filestore/filestore.go b/workhorse/internal/upload/destination/filestore/filestore.go index 2d88874bf25..6b2d8270b51 100644 --- a/workhorse/internal/upload/destination/filestore/filestore.go +++ b/workhorse/internal/upload/destination/filestore/filestore.go @@ -19,3 +19,7 @@ func (lf *LocalFile) Consume(_ context.Context, r io.Reader, _ time.Time) (int64 } return n, err } + +func (lf *LocalFile) ConsumeWithoutDelete(outerCtx context.Context, reader io.Reader, deadLine time.Time) (_ int64, err error) { + return lf.Consume(outerCtx, reader, deadLine) +} diff --git a/workhorse/internal/upload/destination/objectstore/uploader.go b/workhorse/internal/upload/destination/objectstore/uploader.go index 43e573872ee..798a693aa93 100644 --- a/workhorse/internal/upload/destination/objectstore/uploader.go +++ b/workhorse/internal/upload/destination/objectstore/uploader.go @@ -38,11 +38,21 @@ func newETagCheckUploader(strategy uploadStrategy, metrics bool) *uploader { func hexString(h hash.Hash) string { return hex.EncodeToString(h.Sum(nil)) } +func (u *uploader) Consume(outerCtx context.Context, reader io.Reader, deadLine time.Time) (_ int64, err error) { + return u.consume(outerCtx, reader, deadLine, false) +} + +func (u *uploader) ConsumeWithoutDelete(outerCtx context.Context, reader io.Reader, deadLine time.Time) (_ int64, err error) { + return u.consume(outerCtx, reader, deadLine, true) +} + // Consume reads the reader until it reaches EOF or an error. It spawns a // goroutine that waits for outerCtx to be done, after which the remote // file is deleted. The deadline applies to the upload performed inside // Consume, not to outerCtx. -func (u *uploader) Consume(outerCtx context.Context, reader io.Reader, deadline time.Time) (_ int64, err error) { +// SkipDelete optionaly call the Delete() function on the strategy once +// rails is done handling the upload request. +func (u *uploader) consume(outerCtx context.Context, reader io.Reader, deadLine time.Time, skipDelete bool) (_ int64, err error) { if u.metrics { objectStorageUploadsOpen.Inc() defer func(started time.Time) { @@ -59,17 +69,25 @@ func (u *uploader) Consume(outerCtx context.Context, reader io.Reader, deadline // "delete" them. if err != nil { u.strategy.Abort() + + if skipDelete { + // skipDelete avoided the object removal (see the goroutine below). Make + // here that the object is deleted if aborted. + u.strategy.Delete() + } } }() - go func() { - // Once gitlab-rails is done handling the request, we are supposed to - // delete the upload from its temporary location. - <-outerCtx.Done() - u.strategy.Delete() - }() + if !skipDelete { + go func() { + // Once gitlab-rails is done handling the request, we are supposed to + // delete the upload from its temporary location. + <-outerCtx.Done() + u.strategy.Delete() + }() + } - uploadCtx, cancelFn := context.WithDeadline(outerCtx, deadline) + uploadCtx, cancelFn := context.WithDeadline(outerCtx, deadLine) defer cancelFn() var hasher hash.Hash diff --git a/workhorse/internal/upload/destination/upload_opts.go b/workhorse/internal/upload/destination/upload_opts.go index 58427b38b30..d81fa10e97c 100644 --- a/workhorse/internal/upload/destination/upload_opts.go +++ b/workhorse/internal/upload/destination/upload_opts.go @@ -39,6 +39,8 @@ type UploadOpts struct { PresignedPut string // PresignedDelete is a presigned S3 DeleteObject compatible URL. PresignedDelete string + // Whether Workhorse needs to delete the temporary object or not. + SkipDelete bool // HTTP headers to be sent along with PUT request PutHeaders map[string]string // Whether to ignore Rails pre-signed URLs and have Workhorse directly access object storage provider @@ -95,6 +97,7 @@ func GetOpts(apiResponse *api.Response) (*UploadOpts, error) { RemoteURL: apiResponse.RemoteObject.GetURL, PresignedPut: apiResponse.RemoteObject.StoreURL, PresignedDelete: apiResponse.RemoteObject.DeleteURL, + SkipDelete: apiResponse.RemoteObject.SkipDelete, PutHeaders: apiResponse.RemoteObject.PutHeaders, UseWorkhorseClient: apiResponse.RemoteObject.UseWorkhorseClient, RemoteTempObjectID: apiResponse.RemoteObject.RemoteTempObjectID, diff --git a/workhorse/internal/upload/destination/upload_opts_test.go b/workhorse/internal/upload/destination/upload_opts_test.go index a420e842e4d..0fda3bd2381 100644 --- a/workhorse/internal/upload/destination/upload_opts_test.go +++ b/workhorse/internal/upload/destination/upload_opts_test.go @@ -102,6 +102,7 @@ func TestGetOpts(t *testing.T) { MultipartUpload: test.multipart, CustomPutHeaders: test.customPutHeaders, PutHeaders: test.putHeaders, + SkipDelete: true, }, } deadline := time.Now().Add(time.Duration(apiResponse.RemoteObject.Timeout) * time.Second) @@ -111,6 +112,7 @@ func TestGetOpts(t *testing.T) { require.Equal(t, apiResponse.TempPath, opts.LocalTempPath) require.WithinDuration(t, deadline, opts.Deadline, time.Second) require.Equal(t, apiResponse.RemoteObject.ID, opts.RemoteID) + require.Equal(t, apiResponse.RemoteObject.SkipDelete, opts.SkipDelete) require.Equal(t, apiResponse.RemoteObject.GetURL, opts.RemoteURL) require.Equal(t, apiResponse.RemoteObject.StoreURL, opts.PresignedPut) require.Equal(t, apiResponse.RemoteObject.DeleteURL, opts.PresignedDelete) |