Removing legacy remote-settings client#7186
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.
| 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.
There was a problem hiding this comment.
All good. I opened a patch to get the client code updated. I think that will take care of everything.
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.
mergify
bot
dismissed
bendk’s stale review
The pull request has been modified, dismissing previous reviews.
alexcottner
force-pushed
the
rs-client-updates
branch
from
4922dea to
7150195
Compare
|
Rebased after making last change. Opened phab patch. |
bendk
left a comment
bendk
left a comment
There was a problem hiding this comment.
This still looks good to me, re-approving.
mergify
bot
dismissed
bendk’s stale review
The pull request has been modified, dismissing previous reviews.
This reverts commit 64aaa9b.
2 participants
To-Do
Follow-ups
Pull Request checklist