diff options
-rw-r--r-- | .gitlab/issue_templates/Geo Replicate a new Git repository type.md | 84 | ||||
-rw-r--r-- | .gitlab/issue_templates/Geo Replicate a new blob type.md | 95 | ||||
-rw-r--r-- | Gemfile.checksum | 2 | ||||
-rw-r--r-- | Gemfile.lock | 4 | ||||
-rw-r--r-- | app/assets/javascripts/repository/components/tree_content.vue | 17 | ||||
-rw-r--r-- | app/assets/javascripts/repository/constants.js | 6 | ||||
-rw-r--r-- | app/models/ci/bridge.rb | 19 | ||||
-rw-r--r-- | config/feature_flags/development/ci_bridge_remove_sourced_pipelines.yml | 8 | ||||
-rw-r--r-- | locale/gitlab.pot | 3 | ||||
-rw-r--r-- | spec/frontend/repository/components/tree_content_spec.js | 23 | ||||
-rw-r--r-- | spec/frontend/repository/mock_data.js | 9 | ||||
-rw-r--r-- | spec/models/ci/bridge_spec.rb | 22 | ||||
-rw-r--r-- | spec/services/ci/create_downstream_pipeline_service_spec.rb | 71 |
13 files changed, 178 insertions, 185 deletions
diff --git a/.gitlab/issue_templates/Geo Replicate a new Git repository type.md b/.gitlab/issue_templates/Geo Replicate a new Git repository type.md index 571b0db0a30..97f756f0d02 100644 --- a/.gitlab/issue_templates/Geo Replicate a new Git repository type.md +++ b/.gitlab/issue_templates/Geo Replicate a new Git repository type.md @@ -18,7 +18,7 @@ If your Model's pluralized form is non-standard, i.e. it doesn't just end in `s` --> -## Replicate Cool Widgets +## Replicate Cool Widgets - Repository This issue is for implementing Geo replication and verification of Cool Widgets. @@ -39,8 +39,6 @@ You can look into the following example for implementing replication/verificatio ### Modify database schemas to prepare to add Geo support for Cool Widgets -You might do this section in its own merge request, but it is not required. - #### Add the registry table to track replication and verification state Geo secondary sites have a [Geo tracking database](https://gitlab.com/gitlab-org/gitlab/-/blob/master/doc/development/geo.md#tracking-database) independent of the main database. It is used to track the replication and verification state of all replicables. Every Model has a corresponding "registry" table in the Geo tracking database. @@ -114,7 +112,7 @@ Geo secondary sites have a [Geo tracking database](https://gitlab.com/gitlab-org bin/rake db:migrate:geo ``` -- [ ] Be sure to commit the relevant changes in `ee/db/geo/structure.sql` +- [ ] Be sure to commit the relevant changes in `ee/db/geo/structure.sql` and the file under `ee/db/geo/schema_migrations` ### Add verification state to the Model @@ -146,7 +144,7 @@ The Geo primary site needs to checksum every replicable so secondaries can verif t.datetime_with_timezone :verified_at t.references :cool_widget, primary_key: true, default: nil, index: false, foreign_key: { on_delete: :cascade } t.integer :verification_state, default: 0, limit: 2, null: false - t.integer :verification_retry_count, limit: 2 + t.integer :verification_retry_count, default: 0, limit: 2, null: false t.binary :verification_checksum, using: 'verification_checksum::bytea' t.text :verification_failure, limit: 255 @@ -185,7 +183,21 @@ The Geo primary site needs to checksum every replicable so secondaries can verif bin/rake db:migrate ``` -- [ ] Be sure to commit the relevant changes in `db/structure.sql` +- [ ] Be sure to commit the relevant changes in `db/structure.sql` and the file under `db/schema_migrations` + +- [ ] Add an entry for the state table in `db/docs/cool_widget_states.yml` + + ```yaml + --- + table_name: cool_widget_states + classes: + - Geo::CoolWidgetState + feature_categories: + - geo_replication + description: Separate table for cool widget verification states + introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/XXXXX + milestone: 'XX.Y' + ``` That's all of the required database changes. @@ -371,7 +383,6 @@ That's all of the required database changes. ```shell bin/feature-flag --ee geo_cool_widget_replication --type development --group 'group::geo' - bin/feature-flag --ee geo_cool_widget_verification --type development --group 'group::geo' ``` - [ ] Add this replicator class to the method `replicator_classes` in @@ -382,7 +393,6 @@ That's all of the required database changes. ::Geo::PackageFileReplicator, ::Geo::CoolWidgetReplicator ] - end ``` - [ ] Create `ee/spec/replicators/geo/cool_widget_replicator_spec.rb` and perform the necessary setup to define the `model_record` variable for the shared examples: @@ -478,25 +488,29 @@ That's all of the required database changes. end ``` -- [ ] Add the following to `spec/factories/cool_widgets.rb`: +- [ ] Add the following to `ee/spec/factories/cool_widgets.rb`: ```ruby - trait :verification_succeeded do - with_file - verification_checksum { 'abc' } - verification_state { CoolWidget.verification_state_value(:verification_succeeded) } - end + FactoryBot.modify do + trait :verification_succeeded do + with_file + verification_checksum { 'abc' } + verification_state { CoolWidget.verification_state_value(:verification_succeeded) } + end - trait :verification_failed do - with_file - verification_failure { 'Could not calculate the checksum' } - verification_state { CoolWidget.verification_state_value(:verification_failed) } + trait :verification_failed do + with_file + verification_failure { 'Could not calculate the checksum' } + verification_state { CoolWidget.verification_state_value(:verification_failed) } + end end ``` + If there is not an existing factory for the object in `spec/factories/cool_widgets.rb`, wrap the traits in `FactoryBot.create` instead of `FactoryBot.modify`. + - [ ] Make sure the factory also allows setting a `project` attribute. If the model does not have a direct relation to a project, you can use a `transient` attribute. Check out `spec/factories/merge_request_diffs.rb` for an example. -- [ ] Following [the example of Merge Request Diffs](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/63309) add a `Geo::CoolWidgetState` model in `ee/app/models/ee/geo/cool_widget_state.rb`: +- [ ] Following [the example of Merge Request Diffs](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/63309) add a `Geo::CoolWidgetState` model in `ee/app/models/geo/cool_widget_state.rb`: ``` ruby # frozen_string_literal: true @@ -536,6 +550,8 @@ That's all of the required database changes. end ``` +- [ ] Add `[:cool_widget, :remote_store]` and `[:geo_cool_widget_state, any]` to `skipped` in `spec/models/factories_spec.rb` + #### Step 2. Implement metrics gathering Metrics are gathered by `Geo::MetricsUpdateWorker`, persisted in `GeoNodeStatus` for display in the UI, and sent to Prometheus: @@ -556,16 +572,18 @@ Metrics are gathered by `Geo::MetricsUpdateWorker`, persisted in `GeoNodeStatus` - [ ] Add the same fields to `GET /geo_nodes/status` example response in `ee/spec/fixtures/api/schemas/public_api/v4/geo_node_status.json`. - [ ] Add the following fields to the `Sidekiq metrics` table in `doc/administration/monitoring/prometheus/gitlab_metrics.md`: - - `geo_cool_widgets` - - `geo_cool_widgets_checksum_total` - - `geo_cool_widgets_checksummed` - - `geo_cool_widgets_checksum_failed` - - `geo_cool_widgets_synced` - - `geo_cool_widgets_failed` - - `geo_cool_widgets_registry` - - `geo_cool_widgets_verification_total` - - `geo_cool_widgets_verified` - - `geo_cool_widgets_verification_failed` + ```markdown + | `geo_cool_widgets` | Gauge | XX.Y | Number of Cool Widgets on primary | `url` | + | `geo_cool_widgets_checksum_total` | Gauge | XX.Y | Number of Cool Widgets checksummed successfully on primary | `url` | + | `geo_cool_widgets_checksummed` | Gauge | XX.Y | Number of Cool Widgets failed to calculate the checksum on primary | `url` | + | `geo_cool_widgets_checksum_failed` | Gauge | XX.Y | Number of Cool Widgets tried to checksum on primary | `url` | + | `geo_cool_widgets_synced` | Gauge | XX.Y | Number of syncable Cool Widgets synced on secondary | `url` | + | `geo_cool_widgets_failed` | Gauge | XX.Y | Number of syncable Cool Widgets failed to sync on secondary | `url` | + | `geo_cool_widgets_registry` | Gauge | XX.Y | Number of Cool Widgets in the registry | `url` | + | `geo_cool_widgets_verification_total` | Gauge | XX.Y | Number of Cool Widgets verified on secondary | `url` | + | `geo_cool_widgets_verified` | Gauge | XX.Y | Number of Cool Widgets' verifications failed on secondary | `url` | + | `geo_cool_widgets_verification_failed` | Gauge | XX.Y | Number of Cool Widgets' verifications tried on secondary | `url` | + ``` Cool Widget replication and verification metrics should now be available in the API, the `Admin > Geo > Nodes` view, and Prometheus. @@ -731,6 +749,14 @@ As illustrated by the above two examples, batch destroy logic cannot be handled end end ``` + +### Code Review + +When requesting review from database reviewers: + +- [ ] Include a comment mentioning that the change is based on a documented template. +- [ ] `replicables_for_current_secondary` and `available_replicables` may differ per Model. If their queries are new, then add [query plans](https://docs.gitlab.com/ee/development/database_review.html#query-plans) to the MR description. An easy place to gather SQL queries is your GDK's `log/test.log` when running tests of these methods. + ### Release Geo support of Cool Widgets - [ ] In the rollout issue you created when creating the feature flag, modify the Roll Out Steps: diff --git a/.gitlab/issue_templates/Geo Replicate a new blob type.md b/.gitlab/issue_templates/Geo Replicate a new blob type.md index 121dbdf035f..9dfc83309cc 100644 --- a/.gitlab/issue_templates/Geo Replicate a new blob type.md +++ b/.gitlab/issue_templates/Geo Replicate a new blob type.md @@ -18,7 +18,7 @@ If your Model's pluralized form is non-standard, i.e. it doesn't just end in `s` --> -## Replicate Cool Widgets +## Replicate Cool Widgets - Blob This issue is for implementing Geo replication and verification of Cool Widgets. @@ -41,8 +41,6 @@ You can look into the following examples of MRs for implementing replication/ver ### Modify database schemas to prepare to add Geo support for Cool Widgets -You might do this section in its own merge request, but it is not required. - #### Add the registry table to track replication and verification state Geo secondary sites have a [Geo tracking database](https://gitlab.com/gitlab-org/gitlab/-/blob/master/doc/development/geo.md#tracking-database) independent of the main database. It is used to track the replication and verification state of all replicables. Every Model has a corresponding "registry" table in the Geo tracking database. @@ -114,7 +112,7 @@ Geo secondary sites have a [Geo tracking database](https://gitlab.com/gitlab-org bin/rake db:migrate:geo ``` -- [ ] Be sure to commit the relevant changes in `ee/db/geo/structure.sql` +- [ ] Be sure to commit the relevant changes in `ee/db/geo/structure.sql` and the file created under `ee/db/geo/schema_migrations` ### Add verification state fields on the Geo primary site @@ -148,7 +146,7 @@ The Geo primary site needs to checksum every replicable so secondaries can verif t.datetime_with_timezone :verified_at t.references :cool_widget, primary_key: true, default: nil, index: false, foreign_key: { on_delete: :cascade } t.integer :verification_state, default: 0, limit: 2, null: false - t.integer :verification_retry_count, limit: 2 + t.integer :verification_retry_count, default: 0, limit: 2, null: false t.binary :verification_checksum, using: 'verification_checksum::bytea' t.text :verification_failure, limit: 255 @@ -189,7 +187,21 @@ The Geo primary site needs to checksum every replicable so secondaries can verif - [ ] If `cool_widgets` is a high-traffic table, follow [the database documentation to use `with_lock_retries`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/doc/development/migration_style_guide.md#when-to-use-the-helper-method) -- [ ] Be sure to commit the relevant changes in `db/structure.sql` +- [ ] Be sure to commit the relevant changes in `db/structure.sql` and the file under `db/schema_migrations` + +- [ ] Add an entry for the state table in `db/docs/cool_widget_states.yml` + + ```yaml + --- + table_name: cool_widget_states + classes: + - Geo::CoolWidgetState + feature_categories: + - geo_replication + description: Separate table for cool widget verification states + introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/XXXXX + milestone: 'XX.Y' + ``` That's all of the required database changes. @@ -248,7 +260,7 @@ That's all of the required database changes. # @param primary_key_in [Range, CoolWidget] arg to pass to primary_key_in scope # @return [ActiveRecord::Relation<CoolWidget>] everything that should be synced to this node, restricted by primary key - def self.replicables_for_current_secondary(primary_key_in) + def replicables_for_current_secondary(primary_key_in) # This issue template does not help you write this method. # # This method is called only on Geo secondary sites. It is called when @@ -329,7 +341,6 @@ That's all of the required database changes. ```shell bin/feature-flag --ee geo_cool_widget_replication --type development --group 'group::geo' - bin/feature-flag --ee geo_cool_widget_verification --type development --group 'group::geo' ``` - [ ] Add this replicator class to the method `replicator_classes` in @@ -340,7 +351,6 @@ That's all of the required database changes. ::Geo::PackageFileReplicator, ::Geo::CoolWidgetReplicator ] - end ``` - [ ] Create `ee/spec/replicators/geo/cool_widget_replicator_spec.rb` and perform the necessary setup to define the `model_record` variable for the shared examples: @@ -439,22 +449,33 @@ That's all of the required database changes. - [ ] Add the following to `spec/factories/cool_widgets.rb`: ```ruby - trait :verification_succeeded do - with_file - verification_checksum { 'abc' } - verification_state { CoolWidget.verification_state_value(:verification_succeeded) } - end + FactoryBot.modify do + trait :verification_succeeded do + with_file + verification_checksum { 'abc' } + verification_state { CoolWidget.verification_state_value(:verification_succeeded) } + end - trait :verification_failed do - with_file - verification_failure { 'Could not calculate the checksum' } - verification_state { CoolWidget.verification_state_value(:verification_failed) } + trait :verification_failed do + with_file + verification_failure { 'Could not calculate the checksum' } + verification_state { CoolWidget.verification_state_value(:verification_failed) } + end end ``` + If there is not an existing factory for the object in `spec/factories/cool_widgets.rb`, wrap the traits in `FactoryBot.create` instead of `FactoryBot.modify` + + [ ] Make sure the factory supports the `:remote_store` trait. If not, add something like + + ```ruby + trait :remote_store do + file_store { CoolWidget::FileUploader::Store::REMOTE } + end + ``` - [ ] Make sure the factory also allows setting a `project` attribute. If the model does not have a direct relation to a project, you can use a `transient` attribute. Check out `spec/factories/merge_request_diffs.rb` for an example. -- [ ] Following [the example of Merge Request Diffs](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/63309) add a `Geo::CoolWidgetState` model in `ee/app/models/ee/geo/cool_widget_state.rb`: +- [ ] Following [the example of Merge Request Diffs](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/63309) add a `Geo::CoolWidgetState` model in `ee/app/models/geo/cool_widget_state.rb`: ``` ruby # frozen_string_literal: true @@ -494,6 +515,8 @@ That's all of the required database changes. end ``` +- [ ] Add `[:cool_widget, :remote_store]` and `[:geo_cool_widget_state, any]` to `skipped` in `spec/models/factories_spec.rb` + #### Step 2. Implement metrics gathering Metrics are gathered by `Geo::MetricsUpdateWorker`, persisted in `GeoNodeStatus` for display in the UI, and sent to Prometheus: @@ -514,18 +537,21 @@ Metrics are gathered by `Geo::MetricsUpdateWorker`, persisted in `GeoNodeStatus` - [ ] Add the same fields to `GET /geo_nodes/status` example response in `ee/spec/fixtures/api/schemas/public_api/v4/geo_node_status.json`. - [ ] Add the following fields to the `Sidekiq metrics` table in `doc/administration/monitoring/prometheus/gitlab_metrics.md`: - - `geo_cool_widgets` - - `geo_cool_widgets_checksum_total` - - `geo_cool_widgets_checksummed` - - `geo_cool_widgets_checksum_failed` - - `geo_cool_widgets_synced` - - `geo_cool_widgets_failed` - - `geo_cool_widgets_registry` - - `geo_cool_widgets_verification_total` - - `geo_cool_widgets_verified` - - `geo_cool_widgets_verification_failed` - -Cool Widget replication and verification metrics should now be available in the API, the `Admin > Geo > Nodes` view, and Prometheus. + + ```markdown + | `geo_cool_widgets` | Gauge | XX.Y | Number of Cool Widgets on primary | `url` | + | `geo_cool_widgets_checksum_total` | Gauge | XX.Y | Number of Cool Widgets checksummed successfully on primary | `url` | + | `geo_cool_widgets_checksummed` | Gauge | XX.Y | Number of Cool Widgets failed to calculate the checksum on primary | `url` | + | `geo_cool_widgets_checksum_failed` | Gauge | XX.Y | Number of Cool Widgets tried to checksum on primary | `url` | + | `geo_cool_widgets_synced` | Gauge | XX.Y | Number of syncable Cool Widgets synced on secondary | `url` | + | `geo_cool_widgets_failed` | Gauge | XX.Y | Number of syncable Cool Widgets failed to sync on secondary | `url` | + | `geo_cool_widgets_registry` | Gauge | XX.Y | Number of Cool Widgets in the registry | `url` | + | `geo_cool_widgets_verification_total` | Gauge | XX.Y | Number of Cool Widgets verified on secondary | `url` | + | `geo_cool_widgets_verified` | Gauge | XX.Y | Number of Cool Widgets' verifications failed on secondary | `url` | + | `geo_cool_widgets_verification_failed` | Gauge | XX.Y | Number of Cool Widgets' verifications tried on secondary | `url` | + ``` + + Cool Widget replication and verification metrics should now be available in the API, the `Admin > Geo > Nodes` view, and Prometheus. #### Step 3. Implement the GraphQL API @@ -690,6 +716,13 @@ As illustrated by the above two examples, batch destroy logic cannot be handled end ``` +### Code Review + +When requesting review from database reviewers: + +- [ ] Include a comment mentioning that the change is based on a documented template. +- [ ] `replicables_for_current_secondary` and `available_replicables` may differ per Model. If their queries are new, then add [query plans](https://docs.gitlab.com/ee/development/database_review.html#query-plans) to the MR description. An easy place to gather SQL queries is your GDK's `log/test.log` when running tests of these methods. + ### Release Geo support of Cool Widgets - [ ] In the rollout issue you created when creating the feature flag, modify the Roll Out Steps: diff --git a/Gemfile.checksum b/Gemfile.checksum index 1816d46b4dd..aa288566a45 100644 --- a/Gemfile.checksum +++ b/Gemfile.checksum @@ -452,7 +452,7 @@ {"name":"rails","version":"6.1.6.1","platform":"ruby","checksum":"17024921a3913fb341f584542b06adf6bb12977a8b92d5fce093c3996c963686"}, {"name":"rails-controller-testing","version":"1.0.5","platform":"ruby","checksum":"741448db59366073e86fc965ba403f881c636b79a2c39a48d0486f2607182e94"}, {"name":"rails-dom-testing","version":"2.0.3","platform":"ruby","checksum":"b140c4f39f6e609c8113137b9a60dfc2ecb89864e496f87f23a68b3b8f12d8d1"}, -{"name":"rails-html-sanitizer","version":"1.4.3","platform":"ruby","checksum":"2ebba6ad9a0b100f79fda853a46851e7664febe1728223f9734281e0d55940d6"}, +{"name":"rails-html-sanitizer","version":"1.4.4","platform":"ruby","checksum":"895d0c87a2b6623891e85c1d507c7f16acda4e77d94692f537df35ba71398bd5"}, {"name":"rails-i18n","version":"7.0.3","platform":"ruby","checksum":"e3158e98c5332d129fd5131f171ac575eb30dbb8919b21595382b08850cf2bd3"}, {"name":"railties","version":"6.1.6.1","platform":"ruby","checksum":"bafecdf2dcbe4ea44e1ab7081fd797aa87ae9bbcd0f3a4372b662a1b93949733"}, {"name":"rainbow","version":"3.1.1","platform":"ruby","checksum":"039491aa3a89f42efa1d6dec2fc4e62ede96eb6acd95e52f1ad581182b79bc6a"}, diff --git a/Gemfile.lock b/Gemfile.lock index 087d4d8aeec..cfc1503b44a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1150,8 +1150,8 @@ GEM rails-dom-testing (2.0.3) activesupport (>= 4.2.0) nokogiri (>= 1.6) - rails-html-sanitizer (1.4.3) - loofah (~> 2.3) + rails-html-sanitizer (1.4.4) + loofah (~> 2.19, >= 2.19.1) rails-i18n (7.0.3) i18n (>= 0.7, < 2) railties (>= 6.0.0, < 8) diff --git a/app/assets/javascripts/repository/components/tree_content.vue b/app/assets/javascripts/repository/components/tree_content.vue index 4a8f83458f4..f6d6004ba96 100644 --- a/app/assets/javascripts/repository/components/tree_content.vue +++ b/app/assets/javascripts/repository/components/tree_content.vue @@ -2,12 +2,13 @@ import paginatedTreeQuery from 'shared_queries/repository/paginated_tree.query.graphql'; import { createAlert } from '~/flash'; import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; -import { __ } from '~/locale'; import { TREE_PAGE_SIZE, TREE_INITIAL_FETCH_COUNT, TREE_PAGE_LIMIT, COMMIT_BATCH_SIZE, + GITALY_UNAVAILABLE_CODE, + i18n, } from '../constants'; import getRefMixin from '../mixins/get_ref'; import projectPathQuery from '../queries/project_path.query.graphql'; @@ -17,6 +18,7 @@ import FilePreview from './preview/index.vue'; import FileTable from './table/index.vue'; export default { + i18n, components: { FileTable, FilePreview, @@ -142,10 +144,19 @@ export default { } }) .catch((error) => { + let gitalyUnavailableError; + if (error.graphQLErrors) { + gitalyUnavailableError = error.graphQLErrors.find( + (e) => e?.extensions?.code === GITALY_UNAVAILABLE_CODE, + ); + } + const message = gitalyUnavailableError + ? this.$options.i18n.gitalyError + : this.$options.i18n.generalError; createAlert({ - message: __('An error occurred while fetching folder content.'), + message, + captureError: true, }); - throw error; }); }, normalizeData(key, data) { diff --git a/app/assets/javascripts/repository/constants.js b/app/assets/javascripts/repository/constants.js index e194bddcc56..5098053c4f7 100644 --- a/app/assets/javascripts/repository/constants.js +++ b/app/assets/javascripts/repository/constants.js @@ -1,5 +1,6 @@ import { __ } from '~/locale'; +export const GITALY_UNAVAILABLE_CODE = 'unavailable'; export const TREE_PAGE_LIMIT = 1000; // the maximum amount of items per page export const TREE_PAGE_SIZE = 100; // the amount of items to be fetched per (batch) request export const TREE_INITIAL_FETCH_COUNT = TREE_PAGE_LIMIT / TREE_PAGE_SIZE; // the amount of (batch) requests to make @@ -100,3 +101,8 @@ export const LEGACY_FILE_TYPES = [ 'cargo_toml', 'go_mod', ]; + +export const i18n = { + generalError: __('An error occurred while fetching folder content.'), + gitalyError: __('Error: Gitaly is unavailable. Contact your administrator.'), +}; diff --git a/app/models/ci/bridge.rb b/app/models/ci/bridge.rb index 662fb3cffa8..22dcc7c18a0 100644 --- a/app/models/ci/bridge.rb +++ b/app/models/ci/bridge.rb @@ -19,11 +19,6 @@ module Ci belongs_to :project belongs_to :trigger_request - # To be removed upon :ci_bridge_remove_sourced_pipelines feature flag removal - has_many :sourced_pipelines, class_name: "::Ci::Sources::Pipeline", - foreign_key: :source_job_id, - inverse_of: :source_bridge - has_one :downstream_pipeline, through: :sourced_pipeline, source: :pipeline validates :ref, presence: true @@ -89,20 +84,8 @@ module Ci end end - def sourced_pipelines - if Feature.enabled?(:ci_bridge_remove_sourced_pipelines, project) - raise 'Ci::Bridge does not have sourced_pipelines association' - end - - super - end - def has_downstream_pipeline? - if Feature.enabled?(:ci_bridge_remove_sourced_pipelines, project) - sourced_pipeline.present? - else - sourced_pipelines.exists? - end + sourced_pipeline.present? end def downstream_pipeline_params diff --git a/config/feature_flags/development/ci_bridge_remove_sourced_pipelines.yml b/config/feature_flags/development/ci_bridge_remove_sourced_pipelines.yml deleted file mode 100644 index 503e676d4ab..00000000000 --- a/config/feature_flags/development/ci_bridge_remove_sourced_pipelines.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: ci_bridge_remove_sourced_pipelines -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/105708 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/384359 -milestone: '15.7' -type: development -group: group::pipeline authoring -default_enabled: false diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 610503697eb..aec6f2944b1 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -16068,6 +16068,9 @@ msgstr "" msgid "Error: Couldn't load some or all of the changes." msgstr "" +msgid "Error: Gitaly is unavailable. Contact your administrator." +msgstr "" + msgid "Error: No AWS credentials were supplied" msgstr "" diff --git a/spec/frontend/repository/components/tree_content_spec.js b/spec/frontend/repository/components/tree_content_spec.js index 6eea66f1a7d..f694c8e9166 100644 --- a/spec/frontend/repository/components/tree_content_spec.js +++ b/spec/frontend/repository/components/tree_content_spec.js @@ -5,19 +5,25 @@ import FilePreview from '~/repository/components/preview/index.vue'; import FileTable from '~/repository/components/table/index.vue'; import TreeContent from 'jh_else_ce/repository/components/tree_content.vue'; import { loadCommits, isRequested, resetRequestedCommits } from '~/repository/commits_service'; +import waitForPromises from 'helpers/wait_for_promises'; +import { createAlert } from '~/flash'; +import { i18n } from '~/repository/constants'; +import { graphQLErrors } from '../mock_data'; jest.mock('~/repository/commits_service', () => ({ loadCommits: jest.fn(() => Promise.resolve()), isRequested: jest.fn(), resetRequestedCommits: jest.fn(), })); +jest.mock('~/flash'); let vm; let $apollo; +const mockResponse = jest.fn().mockReturnValue(Promise.resolve({ data: {} })); -function factory(path, data = () => ({})) { +function factory(path, appoloMockResponse = mockResponse) { $apollo = { - query: jest.fn().mockReturnValue(Promise.resolve({ data: data() })), + query: appoloMockResponse, }; vm = shallowMount(TreeContent, { @@ -222,4 +228,17 @@ describe('Repository table component', () => { expect(loadCommits.mock.calls).toEqual([['', path, '', 0]]); }); }); + + describe('error handling', () => { + const gitalyError = { graphQLErrors }; + it.each` + error | message + ${gitalyError} | ${i18n.gitalyError} + ${'Error'} | ${i18n.generalError} + `('should show an expected error', async ({ error, message }) => { + factory('/', jest.fn().mockRejectedValue(error)); + await waitForPromises(); + expect(createAlert).toHaveBeenCalledWith({ message, captureError: true }); + }); + }); }); diff --git a/spec/frontend/repository/mock_data.js b/spec/frontend/repository/mock_data.js index c1b5f89c37f..28dd19c29e6 100644 --- a/spec/frontend/repository/mock_data.js +++ b/spec/frontend/repository/mock_data.js @@ -108,3 +108,12 @@ export const blobControlsDataMock = { }, }, }; + +export const graphQLErrors = [ + { + message: '14:failed to connect to all addresses.', + locations: [{ line: 16, column: 7 }], + path: ['project', 'repository', 'paginatedTree'], + extensions: { code: 'unavailable', gitaly_code: 14, service: 'git' }, + }, +]; diff --git a/spec/models/ci/bridge_spec.rb b/spec/models/ci/bridge_spec.rb index 169b00b9c74..e8102c2c1ea 100644 --- a/spec/models/ci/bridge_spec.rb +++ b/spec/models/ci/bridge_spec.rb @@ -21,8 +21,8 @@ RSpec.describe Ci::Bridge, feature_category: :continuous_integration do { trigger: { project: 'my/project', branch: 'master' } } end - it 'has many sourced pipelines' do - expect(bridge).to have_many(:sourced_pipelines) + it 'has one sourced pipeline' do + expect(bridge).to have_one(:sourced_pipeline) end it_behaves_like 'has ID tokens', :ci_bridge @@ -34,24 +34,6 @@ RSpec.describe Ci::Bridge, feature_category: :continuous_integration do expect(bridge).to have_one(:downstream_pipeline) end - describe '#sourced_pipelines' do - subject { bridge.sourced_pipelines } - - it 'raises error' do - expect { subject }.to raise_error RuntimeError, 'Ci::Bridge does not have sourced_pipelines association' - end - - context 'when ci_bridge_remove_sourced_pipelines is disabled' do - before do - stub_feature_flags(ci_bridge_remove_sourced_pipelines: false) - end - - it 'returns the sourced_pipelines association' do - expect(bridge.sourced_pipelines).to eq([]) - end - end - end - describe '#retryable?' do let(:bridge) { create(:ci_bridge, :success) } diff --git a/spec/services/ci/create_downstream_pipeline_service_spec.rb b/spec/services/ci/create_downstream_pipeline_service_spec.rb index 6046f9b8e19..fd978bffacb 100644 --- a/spec/services/ci/create_downstream_pipeline_service_spec.rb +++ b/spec/services/ci/create_downstream_pipeline_service_spec.rb @@ -41,12 +41,6 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute', feature_category subject { service.execute(bridge) } - shared_context 'when ci_bridge_remove_sourced_pipelines is disabled' do - before do - stub_feature_flags(ci_bridge_remove_sourced_pipelines: false) - end - end - context 'when downstream project has not been found' do let(:trigger) do { trigger: { project: 'unknown/project' } } @@ -128,19 +122,6 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute', feature_category expect(pipeline.source_bridge).to be_a ::Ci::Bridge end - context 'when ci_bridge_remove_sourced_pipelines is disabled' do - include_context 'when ci_bridge_remove_sourced_pipelines is disabled' - - it 'creates a new pipeline in a downstream project' do - expect(pipeline.user).to eq bridge.user - expect(pipeline.project).to eq downstream_project - expect(bridge.sourced_pipelines.first.pipeline).to eq pipeline - expect(pipeline.triggered_by_pipeline).to eq upstream_pipeline - expect(pipeline.source_bridge).to eq bridge - expect(pipeline.source_bridge).to be_a ::Ci::Bridge - end - end - it_behaves_like 'logs downstream pipeline creation' do let(:downstream_pipeline) { pipeline } let(:expected_root_pipeline) { upstream_pipeline } @@ -179,31 +160,6 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute', feature_category expect(subject).to be_error expect(subject.message).to eq("Already has a downstream pipeline") end - - context 'when ci_bridge_remove_sourced_pipelines is disabled' do - include_context 'when ci_bridge_remove_sourced_pipelines is disabled' - - before do - bridge.sourced_pipelines.create!( - source_pipeline: bridge.pipeline, - source_project: bridge.project, - project: bridge.project, - pipeline: create(:ci_pipeline, project: bridge.project) - ) - end - - it 'logs an error and exits' do - expect(Gitlab::ErrorTracking) - .to receive(:track_exception) - .with( - instance_of(described_class::DuplicateDownstreamPipelineError), - bridge_id: bridge.id, project_id: bridge.project.id) - .and_call_original - expect(Ci::CreatePipelineService).not_to receive(:new) - expect(subject).to be_error - expect(subject.message).to eq("Already has a downstream pipeline") - end - end end context 'when target ref is not specified' do @@ -237,19 +193,6 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute', feature_category expect(pipeline.source_bridge).to be_a ::Ci::Bridge end - context 'when ci_bridge_remove_sourced_pipelines is disabled' do - include_context 'when ci_bridge_remove_sourced_pipelines is disabled' - - it 'creates a new pipeline in a downstream project' do - expect(pipeline.user).to eq bridge.user - expect(pipeline.project).to eq downstream_project - expect(bridge.sourced_pipelines.first.pipeline).to eq pipeline - expect(pipeline.triggered_by_pipeline).to eq upstream_pipeline - expect(pipeline.source_bridge).to eq bridge - expect(pipeline.source_bridge).to be_a ::Ci::Bridge - end - end - it 'updates the bridge status when downstream pipeline gets processed' do expect(pipeline.reload).to be_failed expect(bridge.reload).to be_failed @@ -301,20 +244,6 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute', feature_category expect(pipeline.source_bridge).to be_a ::Ci::Bridge end - context 'when ci_bridge_remove_sourced_pipelines is disabled' do - include_context 'when ci_bridge_remove_sourced_pipelines is disabled' - - it 'creates a child pipeline in the same project' do - expect(pipeline.builds.map(&:name)).to match_array(%w[rspec echo]) - expect(pipeline.user).to eq bridge.user - expect(pipeline.project).to eq bridge.project - expect(bridge.sourced_pipelines.first.pipeline).to eq pipeline - expect(pipeline.triggered_by_pipeline).to eq upstream_pipeline - expect(pipeline.source_bridge).to eq bridge - expect(pipeline.source_bridge).to be_a ::Ci::Bridge - end - end - it 'updates bridge status when downstream pipeline gets processed' do expect(pipeline.reload).to be_created expect(bridge.reload).to be_success |