Commit 9863ad25 authored by Stan Hu's avatar Stan Hu

Fix usage ping misreporting consolidated object storage settings

When consolidated object storage settings are in use, we internally
modify `Settings` for all the object store types. However,
`Settingslogic` currently assumes the data loaded from a YAML is fixed,
so internally it stores data both as a Hash but also defines an accessor
that wraps each key. This results in two different objects that store
the same data. For example, with CI artifacts:

```
(byebug) Settings.artifacts.object_id
=> 233940
(byebug) Settings['artifacts'].object_id
=> 233960
(byebug) Settings['artifacts'].class
Hash
(byebug) Settings.artifacts.class
Settings
```

Our uploaders use the `Settings.artifacts` format, but the usage ping
uses `Settings['artifacts']`. Both should be equivalent, so we make them
consistent by having consolidated object storage parser assign both.

Relates to
https://gitlab.com/gitlab-org/gitlab/-/issues/212903#note_562162045

Changelog: fixed
parent a053d2da
---
title: Fix usage ping misreporting consolidated object storage settings
merge_request: 60526
author:
type: fixed
......@@ -144,6 +144,11 @@ class ObjectStoreSettings
target_config['remote_directory'] = target_config.delete('bucket')
target_config['consolidated_settings'] = true
section['object_store'] = target_config
# Settingslogic internally stores data as a Hash, but it also
# creates a Settings object for every key. To avoid confusion, we should
# update both so that Settings.artifacts and Settings['artifacts'] return
# the same result.
settings[store_type]['object_store'] = target_config
end
settings
......
......@@ -74,6 +74,7 @@ RSpec.describe ObjectStoreSettings do
expect(settings.artifacts['object_store']['proxy_download']).to be false
expect(settings.artifacts['object_store']['remote_directory']).to eq('artifacts')
expect(settings.artifacts['object_store']['consolidated_settings']).to be true
expect(settings.artifacts).to eq(settings['artifacts'])
expect(settings.lfs['enabled']).to be true
expect(settings.lfs['object_store']['enabled']).to be true
......@@ -83,15 +84,18 @@ RSpec.describe ObjectStoreSettings do
expect(settings.lfs['object_store']['proxy_download']).to be true
expect(settings.lfs['object_store']['remote_directory']).to eq('lfs-objects')
expect(settings.lfs['object_store']['consolidated_settings']).to be true
expect(settings.lfs).to eq(settings['lfs'])
expect(settings.pages['enabled']).to be true
expect(settings.pages['object_store']['enabled']).to be true
expect(settings.pages['object_store']['connection']).to eq(connection)
expect(settings.pages['object_store']['remote_directory']).to eq('pages')
expect(settings.pages['object_store']['consolidated_settings']).to be true
expect(settings.pages).to eq(settings['pages'])
expect(settings.external_diffs['enabled']).to be false
expect(settings.external_diffs['object_store']).to be_nil
expect(settings.external_diffs).to eq(settings['external_diffs'])
end
it 'raises an error when a bucket is missing' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment