Skip to content
Snippets Groups Projects

Add Instance Integrations models

Closed George Koltsov requested to merge georgekoltsov/instance-integrations-models- into master
5 unresolved threads

What does this MR do and why?

In order to support integrations in Cells architecture we need to split existing integrations into group/project integrations (existing integrations table) and instance wide integrations. In this MR we define new instance integration models which are going to be used later on.

It additionally updates existing (currently empty) instance_integrations db table to align it's schema with existing integrations table. We previously created this table trimming any unnecessary columns, but we need them in the end in order to provide a smoother transition. Fewer changes required.

Mentions #474809 (closed)

Migration output

Up
main: == [advisory_lock_connection] object_id: 126140, pg_backend_pid: 82711
main: == 20241011103239 UpdateInstanceIntegrationTable: migrating ===================
main: -- add_column(:instance_integrations, :project_id, :bigint, {:null=>true})
main:    -> 0.0042s
main: -- add_column(:instance_integrations, :group_id, :bigint, {:null=>true})
main:    -> 0.0008s
main: -- add_column(:instance_integrations, :inherit_from_id, :bigint, {:null=>true})
main:    -> 0.0008s
main: -- add_column(:instance_integrations, :instance, :boolean, {:default=>true})
main:    -> 0.0046s
main: -- rename_column(:instance_integrations, :type, :type_new)
main:    -> 0.0154s
main: -- transaction_open?(nil)
main:    -> 0.0000s
main: -- transaction_open?(nil)
main:    -> 0.0000s
main: -- execute("ALTER TABLE instance_integrations\nADD CONSTRAINT project_id_null_constraint\nCHECK ( project_id IS NULL )\nNOT VALID;\n")
main:    -> 0.0045s
main: -- execute("SET statement_timeout TO 0")
main:    -> 0.0004s
main: -- execute("ALTER TABLE instance_integrations VALIDATE CONSTRAINT project_id_null_constraint;")
main:    -> 0.0007s
main: -- execute("RESET statement_timeout")
main:    -> 0.0003s
main: -- transaction_open?(nil)
main:    -> 0.0000s
main: -- transaction_open?(nil)
main:    -> 0.0000s
main: -- execute("ALTER TABLE instance_integrations\nADD CONSTRAINT group_id_null_constraint\nCHECK ( group_id IS NULL )\nNOT VALID;\n")
main:    -> 0.0007s
main: -- execute("ALTER TABLE instance_integrations VALIDATE CONSTRAINT group_id_null_constraint;")
main:    -> 0.0014s
main: -- transaction_open?(nil)
main:    -> 0.0000s
main: -- transaction_open?(nil)
main:    -> 0.0000s
main: -- execute("ALTER TABLE instance_integrations\nADD CONSTRAINT inherit_from_id_null_constraint\nCHECK ( inherit_from_id IS NULL )\nNOT VALID;\n")
main:    -> 0.0014s
main: -- execute("ALTER TABLE instance_integrations VALIDATE CONSTRAINT inherit_from_id_null_constraint;")
main:    -> 0.1876s
main: -- transaction_open?(nil)
main:    -> 0.0000s
main: -- transaction_open?(nil)
main:    -> 0.0000s
main: -- execute("ALTER TABLE instance_integrations\nADD CONSTRAINT instance_is_true_constraint\nCHECK ( instance = TRUE )\nNOT VALID;\n")
main:    -> 0.0015s
main: -- execute("ALTER TABLE instance_integrations VALIDATE CONSTRAINT instance_is_true_constraint;")
main:    -> 0.0013s
main: == 20241011103239 UpdateInstanceIntegrationTable: migrated (0.2865s) ==========

main: == [advisory_lock_connection] object_id: 126140, pg_backend_pid: 82711
ci: == [advisory_lock_connection] object_id: 134180, pg_backend_pid: 83133
ci: == 20241011103239 UpdateInstanceIntegrationTable: migrating ===================
ci: -- add_column(:instance_integrations, :project_id, :bigint, {:null=>true})
ci:    -> 0.0021s
ci: -- add_column(:instance_integrations, :group_id, :bigint, {:null=>true})
ci:    -> 0.0052s
ci: -- add_column(:instance_integrations, :inherit_from_id, :bigint, {:null=>true})
ci:    -> 0.0015s
ci: -- add_column(:instance_integrations, :instance, :boolean, {:default=>true})
ci:    -> 0.0056s
ci: -- rename_column(:instance_integrations, :type, :type_new)
ci:    -> 0.0162s
ci: -- transaction_open?(nil)
ci:    -> 0.0000s
ci: -- transaction_open?(nil)
ci:    -> 0.0000s
ci: -- execute("ALTER TABLE instance_integrations\nADD CONSTRAINT project_id_null_constraint\nCHECK ( project_id IS NULL )\nNOT VALID;\n")
ci:    -> 0.0013s
ci: -- execute("SET statement_timeout TO 0")
ci:    -> 0.0004s
ci: -- execute("ALTER TABLE instance_integrations VALIDATE CONSTRAINT project_id_null_constraint;")
ci:    -> 0.0011s
ci: -- execute("RESET statement_timeout")
ci:    -> 0.0004s
ci: -- transaction_open?(nil)
ci:    -> 0.0000s
ci: -- transaction_open?(nil)
ci:    -> 0.0000s
ci: -- execute("ALTER TABLE instance_integrations\nADD CONSTRAINT group_id_null_constraint\nCHECK ( group_id IS NULL )\nNOT VALID;\n")
ci:    -> 0.0004s
ci: -- execute("ALTER TABLE instance_integrations VALIDATE CONSTRAINT group_id_null_constraint;")
ci:    -> 0.0008s
ci: -- transaction_open?(nil)
ci:    -> 0.0000s
ci: -- transaction_open?(nil)
ci:    -> 0.0000s
ci: -- execute("ALTER TABLE instance_integrations\nADD CONSTRAINT inherit_from_id_null_constraint\nCHECK ( inherit_from_id IS NULL )\nNOT VALID;\n")
ci:    -> 0.0025s
ci: -- execute("ALTER TABLE instance_integrations VALIDATE CONSTRAINT inherit_from_id_null_constraint;")
ci:    -> 0.0006s
ci: -- transaction_open?(nil)
ci:    -> 0.0000s
ci: -- transaction_open?(nil)
ci:    -> 0.0000s
ci: -- execute("ALTER TABLE instance_integrations\nADD CONSTRAINT instance_is_true_constraint\nCHECK ( instance = TRUE )\nNOT VALID;\n")
ci:    -> 0.0008s
ci: -- execute("ALTER TABLE instance_integrations VALIDATE CONSTRAINT instance_is_true_constraint;")
ci:    -> 0.0007s
ci: == 20241011103239 UpdateInstanceIntegrationTable: migrated (0.1041s) ==========

ci: == [advisory_lock_connection] object_id: 134180, pg_backend_pid: 83133
Down
main: == [advisory_lock_connection] object_id: 126200, pg_backend_pid: 81813
main: == 20241011103239 UpdateInstanceIntegrationTable: reverting ===================
main: -- remove_column(:instance_integrations, :project_id, {:if_exists=>true})
main:    -> 0.0217s
main: -- remove_column(:instance_integrations, :group_id, {:if_exists=>true})
main:    -> 0.0035s
main: -- remove_column(:instance_integrations, :inherit_from_id, {:if_exists=>true})
main:    -> 0.0038s
main: -- remove_column(:instance_integrations, :instance, {:if_exists=>true})
main:    -> 0.0056s
main: -- rename_column(:instance_integrations, :type_new, :type)
main:    -> 0.0033s
main: == 20241011103239 UpdateInstanceIntegrationTable: reverted (0.0478s) ==========

main: == [advisory_lock_connection] object_id: 126200, pg_backend_pid: 81813
ci: == [advisory_lock_connection] object_id: 126140, pg_backend_pid: 82231
ci: == 20241011103239 UpdateInstanceIntegrationTable: reverting ===================
ci: -- remove_column(:instance_integrations, :project_id, {:if_exists=>true})
ci:    -> 0.0295s
ci: -- remove_column(:instance_integrations, :group_id, {:if_exists=>true})
ci:    -> 0.0030s
ci: -- remove_column(:instance_integrations, :inherit_from_id, {:if_exists=>true})
ci:    -> 0.0046s
ci: -- remove_column(:instance_integrations, :instance, {:if_exists=>true})
ci:    -> 0.0035s
ci: -- rename_column(:instance_integrations, :type_new, :type)
ci:    -> 0.0028s
ci: == 20241011103239 UpdateInstanceIntegrationTable: reverted (0.0593s) ==========

ci: == [advisory_lock_connection] object_id: 126140, pg_backend_pid: 82231

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before After

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

Edited by George Koltsov

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
1 # frozen_string_literal: true
2
3 class UpdateInstanceIntegrationTable < Gitlab::Database::Migration[2.2]
4 milestone '17.5'
5
6 disable_ddl_transaction!
7
8 def up
9 add_column :instance_integrations, :project_id, :bigint, null: true
10 add_column :instance_integrations, :group_id, :bigint, null: true
11 add_column :instance_integrations, :inherit_from_id, :bigint, null: true
12 add_column :instance_integrations, :instance, :boolean, default: true
13
14 rename_column :instance_integrations, :type, :type_new
  • added 1 commit

    • de94c4e3 - Add Instance Integrations models

    Compare with previous version

    • @.luke may I ask you for an early look of this MR? I followed your suggestions. Updated instance integrations table to have the same schema as intergrations but have put db constraints on foreign key columns to always be null. Will polish it up on monday but wonder what you think so far. Thanks :bow:

    • @georgekoltsov It looks good! It looks like there is something not working out of the box with modules that are prepended, like the EnableSslVerification which is prepended into some integrations including Jenkins.

      When super is called here it's an empty array in the instance integrations model, whereas in the regular models it's an array of Integrations::Field instances, and it causes this problem:

      Integrations::Instance::Jenkins.new.fields
      # TypeError: no implicit conversion from nil to integer
      # from /Users/luke/Sites/gitlab-development-kit/gitlab/app/models/concerns/integrations/enable_ssl_verification.rb:29:in `delete_at'

      It seems like modules that are included are working fine. service_hook is defined in the Jenkins integration through HasWebHook:

      Integrations::Instance::Jenkins.new.service_hook # => nil

      But besides that, the models looked good and I didn't bump into any other problems. I can do things like:

      integration = Integrations::Instance::Slack.new
      integration.save # => true
      
      integration.active = true
      integration.valid? # => false
      integration.errors.full_messages # => ["Webhook can't be blank", "Webhook must be a valid URL"]
      integration.webhook = 'https://example.com'
      integration.save # => true
      
      Integrations::Instance::Slack.count # => 1
      Integrations::Instance::Jira.count # => 0
      Edited by Luke Duncalfe
    • Thanks @.luke I managed to get it to work after a bit of struggle by doing the following

      # app/models/integrations/instance_integration.rb
      
            def self.fields
              superclass.fields
            end

      Without this change, the instance model's .fields method returns an empty array (as you highlighted) even though these fields are defined in the superclass, but calling super on existing fields method doesn't fetch the defined fields from the super class, it still returns an empty array from app/models/integration.rb:201. So I updated the instance model module to fetch fields from superclass instead. Seems to work, but let me know if something's not right.

      I am going to add some tests in a meantime.

    • @.luke should be good for another look in case you spot any issues :bow:

    • @georgekoltsov Thanks, I couldn't spot any extra issues. What you mention in #499482 (comment 2161838709) opens a door potentially to a lot of headaches of some form or another. At least some of which I think are problems no matter which approach we take.

      @bmarjanovic Could you please give this MR its backend maintainer review, and let us know your thoughts on the refactor :bow:

    • Great work @georgekoltsov :100:

      We're having some :apple: pipeline, and I left one core comment before continuing, let me know what do you think :baseball:

    • Please register or sign in to reply
  • George Koltsov requested review from @.luke

    requested review from @.luke

  • Luke Duncalfe removed review request for @.luke

    removed review request for @.luke

  • added 1 commit

    • 36271ac0 - Add Instance Integrations models

    Compare with previous version

  • George Koltsov changed the description

    changed the description

    • note

      Adding specs revealed another issue with another integration - Jira. Running the spec fails with

           ActiveRecord::InvalidForeignKey:
             PG::ForeignKeyViolation: ERROR:  insert or update on table "jira_tracker_data" violates foreign key constraint "fk_c98abcd54c"
             DETAIL:  Key (integration_id)=(114) is not present in table "integrations".

      I will probably leave this for a follow up MR.

    • Same for Redmine

        1) Integrations::Instance::Redmine successfully persists the record
           Failure/Error: connection.public_send(...)
           
           ActiveRecord::InvalidForeignKey:
             PG::ForeignKeyViolation: ERROR:  insert or update on table "issue_tracker_data" violates foreign key constraint "fk_33921c0ee1"
             DETAIL:  Key (integration_id)=(170) is not present in table "integrations".
    • Same for Zentao

      2) Integrations::Instance::Zentao behaves like instance integration successfully persists the record
           Failure/Error: connection.public_send(...)
           
           ActiveRecord::InvalidForeignKey:
             PG::ForeignKeyViolation: ERROR:  insert or update on table "zentao_tracker_data" violates foreign key constraint "fk_rails_84efda7be0"
             DETAIL:  Key (integration_id)=(199) is not present in table "integrations".
    • Please register or sign in to reply
  • added 1 commit

    • 66b8ff81 - Add Instance Integrations models

    Compare with previous version

  • George Koltsov marked this merge request as ready

    marked this merge request as ready

  • George Koltsov changed the description

    changed the description

  • Database migrations (on the main database)

    Migrations included in this change have been executed on gitlab.com data for testing purposes. For details, please see the migration testing pipeline (limited access).

    Migration Type Total runtime Result DB size change
    20241011103239 - UpdateInstanceIntegrationTable Regular 10.6 s :white_check_mark: +0.00 B
    Runtime Histogram for all migrations
    Query Runtime Count
    0 seconds - 0.01 seconds 0
    0.01 seconds - 0.1 seconds 1
    0.1 seconds - 1 second 16
    1 second - 5 seconds 0
    5 seconds - 15 seconds 0
    15 seconds - 5 minutes 0
    5 minutes + 0

    Migration: 20241011103239 - UpdateInstanceIntegrationTable

    • Type: Regular
    • Duration: 10.6 s
    • Database size change: +0.00 B
    Calls Total Time Max Time Mean Time Rows Query
    1 10.1 ms 10.1 ms 10.1 ms 0
    ALTER TABLE "instance_integrations" ADD "instance" boolean DEFAULT TRUE
    1 9.7 ms 9.7 ms 9.7 ms 0
    ALTER TABLE "instance_integrations" ADD "project_id" bigint
    1 4.9 ms 4.9 ms 4.9 ms 0
    ALTER TABLE instance_integrations ADD CONSTRAINT project_id_null_constraint CHECK ( project_id IS NULL ) NOT VALID
    1 2.4 ms 2.4 ms 2.4 ms 0
    ALTER TABLE instance_integrations ADD CONSTRAINT inherit_from_id_null_constraint CHECK ( inherit_from_id IS NULL ) NOT VALID
    1 1.7 ms 1.7 ms 1.7 ms 0
    ALTER TABLE instance_integrations ADD CONSTRAINT instance_is_true_constraint CHECK ( instance = TRUE ) NOT VALID
    1 1.5 ms 1.5 ms 1.5 ms 0
    ALTER TABLE instance_integrations VALIDATE CONSTRAINT inherit_from_id_null_constraint
    1 1.2 ms 1.2 ms 1.2 ms 0
    ALTER TABLE "instance_integrations" ADD "inherit_from_id" bigint
    1 1.2 ms 1.2 ms 1.2 ms 0
    ALTER TABLE instance_integrations VALIDATE CONSTRAINT instance_is_true_constraint
    1 1.1 ms 1.1 ms 1.1 ms 0
    ALTER TABLE instance_integrations VALIDATE CONSTRAINT group_id_null_constraint
    1 0.6 ms 0.6 ms 0.6 ms 0
    ALTER TABLE instance_integrations ADD CONSTRAINT group_id_null_constraint CHECK ( group_id IS NULL ) NOT VALID
    1 0.5 ms 0.5 ms 0.5 ms 0
    ALTER TABLE "instance_integrations" ADD "group_id" bigint
    1 0.5 ms 0.5 ms 0.5 ms 0
    ALTER TABLE "instance_integrations" RENAME COLUMN "type" TO "type_new"
    1 0.4 ms 0.4 ms 0.4 ms 0
    ALTER TABLE instance_integrations VALIDATE CONSTRAINT project_id_null_constraint
    1 0.1 ms 0.1 ms 0.1 ms 1
    SELECT "feature_gates"."key", "feature_gates"."value"  FROM "feature_gates"  WHERE "feature_gates"."feature_key" = $1
    2 0.0 ms 0.0 ms 0.0 ms 2
    SELECT pg_backend_pid()
    1 0.0 ms 0.0 ms 0.0 ms 1
    SELECT $1::regtype::oid
    Histogram for UpdateInstanceIntegrationTable
    Query Runtime Count
    0 seconds - 0.01 seconds 0
    0.01 seconds - 0.1 seconds 1
    0.1 seconds - 1 second 16
    1 second - 5 seconds 0
    5 seconds - 15 seconds 0
    15 seconds - 5 minutes 0
    5 minutes + 0

    Other information

    No other migrations pending on GitLab.com

    Clone details
    Clone ID Clone Created At Clone Data Timestamp Expected Removal Time
    database-testing-3794432-15701349-main 2024-10-15T17:53:37Z 2024-10-15T12:47:19Z 2024-10-16 06:04:03 +0000
    database-testing-3794432-15701349-ci 2024-10-15T17:53:37Z 2024-10-15T16:44:58Z 2024-10-16 06:04:03 +0000

    Job artifacts

    Database migrations (on the ci database)

    Migrations included in this change have been executed on gitlab.com data for testing purposes. For details, please see the migration testing pipeline (limited access).

    Migration Type Total runtime Result DB size change
    20241011103239 - UpdateInstanceIntegrationTable Regular 12.3 s :white_check_mark: +0.00 B
    Runtime Histogram for all migrations
    Query Runtime Count
    0 seconds - 0.01 seconds 0
    0.01 seconds - 0.1 seconds 2
    0.1 seconds - 1 second 14
    1 second - 5 seconds 0
    5 seconds - 15 seconds 0
    15 seconds - 5 minutes 0
    5 minutes + 0

    Migration: 20241011103239 - UpdateInstanceIntegrationTable

    • Type: Regular
    • Duration: 12.3 s
    • Database size change: +0.00 B
    Calls Total Time Max Time Mean Time Rows Query
    1 3.5 ms 3.5 ms 3.5 ms 0
    ALTER TABLE "instance_integrations" ADD "project_id" bigint
    1 2.7 ms 2.7 ms 2.7 ms 0
    ALTER TABLE "instance_integrations" ADD "instance" boolean DEFAULT TRUE
    1 0.6 ms 0.6 ms 0.6 ms 0
    ALTER TABLE instance_integrations ADD CONSTRAINT project_id_null_constraint CHECK ( project_id IS NULL ) NOT VALID
    1 0.5 ms 0.5 ms 0.5 ms 0
    ALTER TABLE instance_integrations ADD CONSTRAINT group_id_null_constraint CHECK ( group_id IS NULL ) NOT VALID
    1 0.5 ms 0.5 ms 0.5 ms 0
    ALTER TABLE instance_integrations ADD CONSTRAINT instance_is_true_constraint CHECK ( instance = TRUE ) NOT VALID
    1 0.5 ms 0.5 ms 0.5 ms 0
    ALTER TABLE instance_integrations VALIDATE CONSTRAINT project_id_null_constraint
    1 0.5 ms 0.5 ms 0.5 ms 0
    ALTER TABLE instance_integrations ADD CONSTRAINT inherit_from_id_null_constraint CHECK ( inherit_from_id IS NULL ) NOT VALID
    1 0.4 ms 0.4 ms 0.4 ms 0
    ALTER TABLE instance_integrations VALIDATE CONSTRAINT group_id_null_constraint
    1 0.4 ms 0.4 ms 0.4 ms 0
    ALTER TABLE "instance_integrations" ADD "group_id" bigint
    1 0.4 ms 0.4 ms 0.4 ms 0
    ALTER TABLE instance_integrations VALIDATE CONSTRAINT instance_is_true_constraint
    1 0.4 ms 0.4 ms 0.4 ms 0
    ALTER TABLE "instance_integrations" ADD "inherit_from_id" bigint
    1 0.4 ms 0.4 ms 0.4 ms 0
    ALTER TABLE "instance_integrations" RENAME COLUMN "type" TO "type_new"
    1 0.3 ms 0.3 ms 0.3 ms 0
    ALTER TABLE instance_integrations VALIDATE CONSTRAINT inherit_from_id_null_constraint
    2 0.0 ms 0.0 ms 0.0 ms 2
    SELECT pg_backend_pid()
    1 0.0 ms 0.0 ms 0.0 ms 1
    SELECT $1::regtype::oid
    Histogram for UpdateInstanceIntegrationTable
    Query Runtime Count
    0 seconds - 0.01 seconds 0
    0.01 seconds - 0.1 seconds 2
    0.1 seconds - 1 second 14
    1 second - 5 seconds 0
    5 seconds - 15 seconds 0
    15 seconds - 5 minutes 0
    5 minutes + 0

    Other information

    No other migrations pending on GitLab.com

    Clone details
    Clone ID Clone Created At Clone Data Timestamp Expected Removal Time
    database-testing-3794432-15701349-main 2024-10-15T17:53:37Z 2024-10-15T12:47:19Z 2024-10-16 06:04:03 +0000
    database-testing-3794432-15701349-ci 2024-10-15T17:53:37Z 2024-10-15T16:44:58Z 2024-10-16 06:04:03 +0000

    Job artifacts


    Brought to you by gitlab-org/database-team/gitlab-com-database-testing. Epic

    Edited by ****
  • George Koltsov requested review from @.luke

    requested review from @.luke

  • requested review from @igor.drozdov

  • changed milestone to %17.6

  • mentioned in issue #499482 (closed)

  • Luke Duncalfe approved this merge request

    approved this merge request

  • added pipelinetier-2 label and removed pipelinetier-1 label

  • Before you set this MR to auto-merge

    This merge request will progress on pipeline tiers until it reaches the last tier: pipelinetier-3. We will trigger a new pipeline for each transition to a higher tier.

    Before you set this MR to auto-merge, please check the following:

    • You are the last maintainer of this merge request
    • The latest pipeline for this merge request is pipelinetier-3 (You can find which tier it is in the pipeline name)
    • This pipeline is recent enough (created in the last 8 hours)

    If all the criteria above apply, please set auto-merge for this merge request.

    See pipeline tiers and merging a merge request for more details.

  • Luke Duncalfe removed review request for @.luke

    removed review request for @.luke

  • Luke Duncalfe requested review from @bmarjanovic

    requested review from @bmarjanovic

  • :tools: Generated by gitlab_quality-test_tooling.


    :snail: Slow tests detected in this merge request. These slow tests might be related to this merge request's changes.

    Click to expand
    Job File Name Duration Expected duration
    #8106543841 spec/lib/gitlab/database/decomposition/migrate_spec.rb#L103 Gitlab::Database::Decomposition::Migrate#process! when the checks pass copies main database to ci database 28.76 s < 27.12 s
    #8106544075 spec/features/admin/integrations/instance_integrations_spec.rb#L12 Instance integrations behaves like integration settings form displays all the integrations 114.74 s < 50.13 s
    #8106544024 spec/features/admin/integrations/instance_integrations_spec.rb#L12 Instance integrations behaves like integration settings form displays all the integrations 121.91 s < 50.13 s
    #8106544135 spec/features/admin/integrations/instance_integrations_spec.rb#L12 Instance integrations behaves like integration settings form displays all the integrations 132.82 s < 50.13 s
  • A deleted user added rspec:slow test detected label
  • E2E Test Result Summary

    allure-report-publisher generated test report!

    e2e-test-on-gdk: :white_check_mark: test report for 66b8ff81

    expand test summary
    +------------------------------------------------------------------+
    |                          suites summary                          |
    +-------------+--------+--------+---------+-------+-------+--------+
    |             | passed | failed | skipped | flaky | total | result |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Plan        | 76     | 0      | 0       | 0     | 76    | ✅     |
    | Govern      | 71     | 0      | 2       | 0     | 73    | ✅     |
    | Create      | 128    | 0      | 18      | 0     | 146   | ✅     |
    | Verify      | 45     | 0      | 2       | 0     | 47    | ✅     |
    | Monitor     | 8      | 0      | 0       | 0     | 8     | ✅     |
    | Release     | 5      | 0      | 0       | 0     | 5     | ✅     |
    | Analytics   | 2      | 0      | 0       | 0     | 2     | ✅     |
    | Package     | 17     | 0      | 18      | 0     | 35    | ✅     |
    | Data Stores | 33     | 0      | 1       | 0     | 34    | ✅     |
    | Fulfillment | 2      | 0      | 0       | 0     | 2     | ✅     |
    | Secure      | 4      | 0      | 0       | 0     | 4     | ✅     |
    | Manage      | 1      | 0      | 1       | 0     | 2     | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Total       | 392    | 0      | 42      | 0     | 434   | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+
  • 1 1 # frozen_string_literal: true
    • suggestion: instead of creating 50+ new identical classes, could we do something like the following instead?

      module Integrations
        module Instance
          INTEGRATIONS = %w[AppleAppStore Asana Bamboo ...].freeze
      
          INTEGRATIONS.each do |integration_name|
            const_set(integration_name, Class.new(Object.const_get("::Integrations::#{integration_name}")) do
              include InstanceIntegration
            end)
          end
        end
      end
    • I think we shouldn't do metaprogramming here :slight_smile:. Where we've done metaprogramming to create classes, like in our GraphQL Global ID types I've found over time it's led to problems that would be easy to solve if they existed as normal files, allowing us to treat each one individually if we wanted to in future.

    • @.luke I thought it would be easier to have overall control here, rather than creating 50+ integrations.

      If we have a need to introduce some changes to a single class, we can always create a new one, and remove it from the Integrations::Instance::INTEGRATIONS constant.

    • Please register or sign in to reply
  • Igor Drozdov approved this merge request

    approved this merge request

  • Igor Drozdov removed review request for @igor.drozdov

    removed review request for @igor.drozdov

  • George Koltsov mentioned in merge request !169768 (closed)

    mentioned in merge request !169768 (closed)

  • Due to the issue discovered in #499482 (closed) we no longer are going to follow this approach. Closing this MR

  • Hello @georgekoltsov :wave:

    The database team is looking for ways to improve the database review process and we would love your help!

    If you'd be open to someone on the database team reaching out to you for a chat, or if you'd like to leave some feedback asynchronously, just post a reply to this comment mentioning:

    @gitlab-org/database-team

    And someone will be by shortly!

    Thanks for your help! :heart:

    This message was generated automatically. You're welcome to improve it.

  • Please register or sign in to reply
    Loading