Skip to content
This repository has been archived by the owner on Apr 6, 2023. It is now read-only.

Integrate remote-settings-client #232

Merged
merged 5 commits into from
Dec 6, 2021

Conversation

Dexterp37
Copy link
Contributor

@Dexterp37 Dexterp37 commented Nov 24, 2021

This fixes #221 and contributes to #218 .

This does not currently enable signature verification, but exclusively replaces the custom rs implementation with the remote-settings-client one.

The Kinto server was not properly configured
to track changes on non-main buckets. In tests
we created collections and buckets on the fly
and changes to these were not properly tracked.
By creating collections on the fly exclusively
in the 'main' bucket we make sure that any change
is tracked by Kinto and that everything works
as expected
merino-adm/src/remote_settings/mod.rs Outdated Show resolved Hide resolved
.unwrap_or(&settings.remote_settings.default_collection)
.clone(),
)?;
let mut remote_settings_client = remote_settings_client::Client::builder()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously we were utilizing a FileStorage, e.g. remote_settings_client.storage(Box::new(FileStorage.., do we want to do that again? cc @mythmon

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if we don't we'll be re-downloading attachments at every sync.

merino-adm/src/remote_settings/reqwest_client.rs Outdated Show resolved Hide resolved
merino-adm/src/remote_settings/reqwest_client.rs Outdated Show resolved Hide resolved
Comment on lines 241 to 243
// remote settings behave as expected. We use PUT here to overwrite
// the bucket transparently each time, since we don't really care
// about the stored metadata.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will calling PUT here delete the underlying collections? This method gets called as a part of parallel test runs, so we don't want to delete the collections used by other tests. The integration tests are one of the primary users of /dev/kinto.ini, so we can change that to support the old method, if that's possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I restored the POST

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will calling PUT here delete the underlying collections?

No it won't

@Dexterp37 Dexterp37 merged commit 385e220 into mozilla-services:main Dec 6, 2021
@Dexterp37 Dexterp37 deleted the rs-client branch December 6, 2021 18:04
mythmon added a commit that referenced this pull request Dec 8, 2021
* Revert "Merge pull request #232 from Dexterp37/rs-client" - Integrate
  remote-settings-client
* Revert "Merge pull request #260 from mozilla-services/kinto-url-with-v1" -
  Include `/v1` on the end of the remote settings URL we give to the RS client
* Revert "Merge pull request #261 from mozilla-services/feat/259"
  don't hide inner errors in the log message
* Revert "Merge pull request #262 from mozilla-services/gh-259"
  Fix intermittent failures with ReqwestClient
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate remote-settings-client in Merino (in a branch)
4 participants