Bump Ubuntu Version in GH Actions (Close #85) #86
Conversation
snowplowcla
added
the
cla:yes
| BinaryFormatter binFormatter = new BinaryFormatter(); | ||
| MemoryStream mStream = new MemoryStream(); | ||
| binFormatter.Serialize(mStream, dict); | ||
| MessagePackSerializer.Serialize(mStream, dict); |
There was a problem hiding this comment.
Is the binary format of BinaryFormatter and MessagePackSerializer compatible? I assume it's not, so that would mean this is a breaking change as the serialized session information won't be readable anymore (we might need to add some tests to see what happens if MessagePackSerializer tries to read a binary file serialized using BinaryFormatter).
Is there no way we can keep the deprecated BinaryFormatter?
There was a problem hiding this comment.
The issue with the BinaryFormatter seems to be that its insecure as it will work on any data type. There's more info here, but it sounds like there's no option to keep using it.
I suppose given we've not reached a major release for the Unity tracker a breaking change isnt the end of the world - but adding some tests to check the differences is probably sensible
There was a problem hiding this comment.
Yeah, I agree the tracker deserves a major version, but that increases the scope of this task much more.
I have tried to run the Tests project in Unity Hub and now instead of just including the dll for SnowplowTracker and LiteDB, I had to extra 5 dlls for MessagePack and it's dependencies. Since we currently don't publish the tracker through a package repository, this complicates the installation process quite a bit for users. I'd prefer if instead of using MessagePack, we used something built into .net like JSON serialization.
If we are doing a major release, we should also do a few more updates, like finally renaming Unstructured to SelfDescribing and publishing to a package manager. We also had a plan to unify the .NET and Unity trackers so that we don't have to maintain them separately.
I think we don't have the capacity to do this now and frankly fixing the CI actions is not a good enough reason to rush a major version out – it doesn't break anything for our users, it's just the CI that is broken. So I would suggest delaying this work into a proper new major version of the tracker which we can plan what it should contain.
There was a problem hiding this comment.
As discussed - I've just stuck to updating the CI action, we can do the major version once we have capacity!
There was a problem hiding this comment.
I'll update the release branch name too
Jack-Keene
force-pushed
the
issue/85-bump-ubuntu-version
branch
from
825a4a1 to
e7cb7bd
Compare
Jack-Keene
force-pushed
the
issue/85-bump-ubuntu-version
branch
from
e7cb7bd to
b2fd5c9
Compare
matus-tomlein
left a comment
matus-tomlein
left a comment
There was a problem hiding this comment.
LGTM, please make a task to update the .NET version and replace BinaryFomatter so that we don't forget!
No description provided.