Removing legacy remote-settings client#7186
Removing legacy remote-settings client#7186alexcottner wants to merge 7 commits intomozilla:mainfrom
Conversation
alexcottner
force-pushed
the
rs-client-updates
branch
5 times, most recently
from
b25281a to
211031e
Compare
alexcottner
force-pushed
the
rs-client-updates
branch
5 times, most recently
from
45266e4 to
5a8684c
Compare
alexcottner
changed the title
bendk
left a comment
bendk
left a comment
There was a problem hiding this comment.
This is looking good overall, but I had a couple suggestions. Also, I think the branch needs a rebase or something, because there's some extra commits here that don't seem like they belong.
There was a problem hiding this comment.
I'm confused by this one, wasn't this already merged?
| pub server: Option<RemoteSettingsServer>, | ||
| } | ||
| // temporary alias for compatability | ||
| pub type RemoteSettingsConfig2 = RemoteSettingsConfig; |
There was a problem hiding this comment.
Is anyone using this? It seems like this could only affect Rust consumers since it's not exported via UniFFI, but those are all getting updated by this PR right?
There was a problem hiding this comment.
Ah, perhaps I'm misunderstaning something. But I found this reference in kotlin in firefox and thought I would need a temporary alias to transition things.
There was a problem hiding this comment.
I think your reference is correct, but I don't think the type alias will prevent the breaking change. For that to happen, UniFFI would also need to create a type alias in the generated code and I don't think there's a good way to do that currently.
I think the best solution is to just accept the breakage and create PRs for iOS and Desktop/Android that replaces RemoteSettingsConfig2 with RemoteSetttingsConfig. Sorry about this step, it's pretty annoying, but the replacements should be pretty straightforward at least.
| ); | ||
| Ok(()) | ||
| } else { | ||
| Err(Error::MissingRecords) |
There was a problem hiding this comment.
If this is the only place where we're returning MissingRecords, then I don't think we need a new variant for it. I think it's safe to just call unwrap(), since get_records should always return a Some value when sync_if_empty=true is passed.
alexcottner
force-pushed
the
rs-client-updates
branch
from
e2a9d13 to
94c68be
Compare
mergify
bot
dismissed
bendk’s stale review
The pull request has been modified, dismissing previous reviews.
|
Rebased and adjusted based on PR feedback. Still have an open question on the config alias. |
bendk
left a comment
bendk
left a comment
There was a problem hiding this comment.
The changes in this repo look good to me, so I'm approving this one. However, don't merge it before you have PRs ready for the other repos since I don't think there's a way to avoid making this a breaking change.
|
|
||
| ### Remote-Settings | ||
| * Removed old remote-settings client code that is no longer used. | ||
| * Started renaming `RemoteSettingsConfig2` to `RemoteSettingsConfig`, leaving an alias for the prior struct |
There was a problem hiding this comment.
Assuming I'm right about the alias, you'll need to update this line.
2 participants
To-Do
Follow-ups
Pull Request checklist