Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backward compat for deserialization of InstalledAppCommon #2253

Merged
merged 5 commits into from
Apr 21, 2023

Conversation

maackle
Copy link
Member

@maackle maackle commented Apr 19, 2023

Summary

Backport to fix backward compat breakage

TODO:

  • CHANGELOG(s) updated with appropriate info
  • Just before pressing the merge button, ensure new entries to CHANGELOG(s) are still under the UNRELEASED heading

@maackle maackle changed the base branch from develop to fix-0.1-breaks April 19, 2023 20:04
@maackle maackle changed the base branch from fix-0.1-breaks to develop April 19, 2023 20:08
@ThetaSinner ThetaSinner self-requested a review April 19, 2023 20:14
@ThetaSinner
Copy link
Contributor

Would it be possible to write a test to verify that the struct can initialise correctly from previous data?

Does this need to go into develop at all if it can be removed later? Could this change go directly into develop-0.1 and only live on the 0.1 stream. From my point of view I don't mind needing to clean out any sandbox environments when I pull a new version of develop, it's helpful to know it's coming but beyond that I think it's fine

@maackle
Copy link
Member Author

maackle commented Apr 19, 2023

@ThetaSinner it would be pretty cumbersome to write that test, I think. We'd need a fixture database including a serialized struct produced by the old version, and load it in the new version. Might be a good idea, and it would help for all kinds of other changes, but it's hard to feel motivated to do that right now given that we could have benefited from it a long time ago and could do it at any time.

And yes I guess it does make sense to just merge this into the 0.1 stream

@maackle maackle changed the base branch from develop to develop-0.1 April 19, 2023 20:21
@ThetaSinner
Copy link
Contributor

That makes sense, yeah it's not worth it if it's going to require a whole new kind of test and the pieces to go with it. We can just check that the release candidate that this goes out on behaves as expected. I was thinking you might get away with embedding some sample JSON in a test and deserialising it with serde_json but even getting the data out of a live database and into JSON isn't a trivial operation due to encryption and binary storage

@maackle
Copy link
Member Author

maackle commented Apr 19, 2023

Yeah, we could construct some JSON that's supposed to represent the old format, but it seems just as likely that we would get that wrong as it is that we would make a mistake with compatibility. Seems that the only reliable test would be one based on real data from real serialization.

@maackle maackle enabled auto-merge (squash) April 20, 2023 23:26
@ThetaSinner ThetaSinner enabled auto-merge (squash) April 21, 2023 08:58
@ThetaSinner ThetaSinner merged commit 007defa into develop-0.1 Apr 21, 2023
12 checks passed
@ThetaSinner ThetaSinner deleted the fix-app-info-manifest branch April 21, 2023 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants