Sync changes in ApplicationSetting to Default Organization

What does this MR do and why?

In MR !123380 (merged), we added one ApplicationSetting (restricted_visibility_levels) to OrganizationSetting model.

This MR will sync changes in ApplicationSetting to OrganizationSetting. This is needed because we do not have a UI for OrganizationSettings.

Implementation

• Sync logic is in ApplicationSettings::SyncService
• It is now called from the ApplicationSettings::UpdaterService which is used in a few places, including the controller that handles updates from the UI

We have a few database migrations that are modifying ApplicationSetting without using this UpdaterService or the ActiveRecord model. Those changes are missed in this MR. At some point, we will start to remove settings from ApplicationSetting so I think it is acceptable. New settings need to be added either to ApplicationSetting (instance level) or OrganzationSetting (organization level).

We could also consider adding a job that runs this service periodically.

How to set up and validate locally

1. Open rails console and delete all OrganizationSettings
2. http://localhost:3000/admin/application_settings/general
   • Expand Visibility and access controls
   • Edit Restricted Visibility levels and save
3. In rails console:
   • ApplicationSetting.first.restriced_visiblity_levels should be updated
   • Organizations::Organization.default_organization.settings.restricted_visibility_levels should be updated and match ApplicationSetting

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

• I have evaluated the MR acceptance checklist for this MR.

Related to #394801 (closed)

changed milestone to %16.2

added Category:Groups & Projects Category:Organization backend devopsdata stores grouptenant scale milestonep1 missed:16.1 sectioncore platform typefeature workflowin dev labels

assigned to @rutgerwessels

removed Category:Groups & Projects label

removed missed:16.1 label

	1 Warning
	Please add a merge request subtype to this merge request.

	1 Message
	CHANGELOG missing: If you want to create a changelog entry for GitLab FOSS, add the Changelog trailer to the commit message you want to add to the changelog. If you want to create a changelog entry for GitLab EE, also add the EE: true trailer to your commit message. If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.

Reviewer roulette

Changes that require review have been detected!

Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:

Category	Reviewer	Maintainer
backend	Harsimar Sandhu (@harsimarsandhu) (UTC+5.5, 3.5 hours ahead of @rutgerwessels)	Sincheol (David) Kim (@dskim_gitlab) (UTC+9.5, 7.5 hours ahead of @rutgerwessels)

To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.

To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.

Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.

If needed, you can retry the danger-review job that generated this comment.

Generated by Danger

Allure report

allure-report-publisher generated test report!

e2e-test-on-gdk: test report for fe67f393

expand test summary

+-----------------------------------------------------------------------+
|                            suites summary                             |
+------------------+--------+--------+---------+-------+-------+--------+
|                  | passed | failed | skipped | flaky | total | result |
+------------------+--------+--------+---------+-------+-------+--------+
| Monitor          | 4      | 0      | 0       | 4     | 4     | ❗     |
| Data Stores      | 2      | 0      | 0       | 0     | 2     | ✅     |
| Create           | 8      | 0      | 1       | 6     | 9     | ❗     |
| Plan             | 4      | 0      | 0       | 4     | 4     | ❗     |
| Manage           | 1      | 0      | 0       | 0     | 1     | ✅     |
| Govern           | 2      | 0      | 0       | 0     | 2     | ✅     |
| Framework sanity | 0      | 0      | 1       | 0     | 1     | ➖     |
+------------------+--------+--------+---------+-------+-------+--------+
| Total            | 21     | 0      | 2       | 14    | 23    | ❗     |
+------------------+--------+--------+---------+-------+-------+--------+

@rutgerwessels we will probably need to do this lots of times as each setting will run through the same flow. Would be great if you could build the mechanism in a generic way for re-use

@alexpooley I am refactoring this into a separate service object so it is decoupled from the models. So it can be called in different code paths or even from Sidekiq worker and not only after_commit hook

@alexpooley I came up with this solution. What do you think? Can you do initial backend review?

@rutgerwessels I'm not sure if we want to sync to all organizations, not just the default org? Say there's no UI for a setting then we want to push the setting change down to all organisations?

As you suggest above, the value of the sync mechanism is to alleviate problems that arise from mismatched development schedules. So first we get the setting added in the model, but then maybe we need to backfill the setting, build/amend queries, build the UI form fields, and so on.

Also we will eventually need an explicit settings sync list, or a way to exclude fields from the sync. I also wonder if some settings will be overridable by the instance level but possibly a problem for later.

@alexpooley We do not want to sync instance level settings to all organizations. ApplicationSettings will be 'instance' level. So in the end, we will have 1 set of ApplicationSettings for each instance. And multiple Organization-level settings.

So this MR will not sync to multiple organizations. This MR is only taking care of syncing settings from ApplicationSettings to the settings of the Default Organization. I can rename the service class to reflect that (ApplicationSettins::SyncToDefaultOrganizationService), do you think that is needed? This is a bit of temporary code until we migrated the subset of settings to new OrganizationSetting UI.

This MR is a step forward in the direction of moving settings from Instance level to Organization Level. As long as we do not have a UI for Organizations, we will not have multiple organizations. A next MR will return the settings from either ApplicationSettings or OrganizationSettings. I follow this approach

This MR will give us this, when a administrator modifies settings:

• After saving the ApplicationSettings, it will create a list of attributes that are supported by both models (in this MR: restricted_visibility_levels)
• It will then store those attributes in OrganizationSetting for the Default Organization

I did not include backfilling, do you think it should be part of this MR? It is not a difficult one (just one record).

This is a bit of temporary code until we migrated the subset of settings to new OrganizationSetting UI.

My concern is that there's a lot of settings to migrate and they will all be in various stages of development over many milestones. We might not have everything migrated by the time we create more organizations?

@rutgerwessels I think I'm a bit unclear on the motivation for the MR.

This MR will sync changes in ApplicationSetting to OrganizationSetting. This is needed because we do not have a UI for OrganizationSettings.

What happens to this work when we have a UI for OrganizationSettings?

@alexpooley When we have an UI for OrganizationSettings, we can drop the column from ApplicationSetting and it will not sync anymore

I expect a transition that will migrate settings in groups. So will probably move related settings (from UX perspective) over, in groups of 5-10 settings.

@rutgerwessels I'm not clear why the default org is special here. Does the MR assume that all settings will be migrated to org settings before we create a second organization?

@alexpooley It is assuming that we could migrate a few settings before we have a second organization and / or a UI. Until that time, we consider the settings in the Default Org as a replacement for the ApplicationSettings. So we can already start moving settings over. For example, this service allows us to store restricted_visibility_levels in both ApplicationSettings and OrganizationSettings.

But now I am leaning towards closing this MR. If we want to move a few settings, we can do this:

• Port validations and other supporting code from ApplicationSetting to OrganizationSetting
• Add UI for these settings (using new Organization UI)
• Data migration: copy the values for the to-be-migrated settings to all existing Organizations?
• Drop the column

In that case, we don't need a 'sync service'

What do you think?

@rutgerwessels The four point plan you laid out is what I had in mind. There's a little feature flag dance needed somewhere between those steps to swap from old to new. There's a potential race condition where a new Org is created, and it needs to pull it's org settings from the app settings. This is where I thought your sync would be useful. This is not so much a problem now but potentially in the future when we don't have tight control on org creation timing.

added 628 commits

• 3d5b279f...b047402d - 626 commits from branch master
• f08db397 - Add Service for syncing Settings
• cb569a66 - When changes are updated, sync to OrganizationSettings

Compare with previous version

added 1 commit

• fe67f393 - When changes are updated, sync to OrganizationSettings

Compare with previous version

marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed

added featureenhancement label

marked this merge request as ready

changed the description

requested review from @alexpooley

mentioned in issue #394801 (closed)

I am closing this because I think this is dependent on the how exactly we are going to migrate which is part of how we rollout organizations

closed

mentioned in issue #419543

added devopstenant scale grouporganizations sectioninfrastructure platforms labels and removed devopsdata stores grouptenant scale [DEPRECATED] sectioncore platform labels