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

Update settings schema to V4 to fix a migration bug in obfuscation settings #5543

Conversation

rablador
Copy link
Contributor

@rablador rablador commented Dec 1, 2023

We've introduced breaking changes in the current main that necessitate a new settings schema version. We should add a migration and bump the schema version for this. This can currently be verified by installing a main version over 2023.7.


This change is Reviewable

@rablador rablador added the iOS Issues related to iOS label Dec 1, 2023
Copy link
Collaborator

@mojganii mojganii left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @rablador)


ios/MullvadSettings/TunnelSettings.swift line 31 at r1 (raw file):

    case v3 = 3

    /// V3 format with support for WireGuard obfuscation options with UInt16 port backing values,

I would suggest to consider more descriptive comments for these sort of schemas .I'm confused when I read them...
V4 supports V3 configuration plus back port abilities for WireGuard obfuscation

@rablador rablador force-pushed the add-another-settings-schema-version-due-to-changes-in-how-ios-403 branch 2 times, most recently from 4e2ff38 to 738dda4 Compare December 4, 2023 10:26
Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

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

:shipit: !

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rablador)

Copy link
Contributor Author

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Dismissed @mojganii from a discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


ios/MullvadSettings/TunnelSettings.swift line 31 at r1 (raw file):

Previously, mojganii wrote…

I would suggest to consider more descriptive comments for these sort of schemas .I'm confused when I read them...
V4 supports V3 configuration plus back port abilities for WireGuard obfuscation

Reverted version changes altogether, so I'm marking this comment as resolved.

@rablador rablador force-pushed the add-another-settings-schema-version-due-to-changes-in-how-ios-403 branch from 738dda4 to 31de7a1 Compare December 4, 2023 12:30
Copy link
Contributor Author

@rablador rablador left a comment

Choose a reason for hiding this comment

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

After IRL discussion it was decided to go for a smaller change without version bump to tunnel settings.

Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @buggmagnet)

@buggmagnet buggmagnet merged commit 12dc193 into main Dec 4, 2023
@buggmagnet buggmagnet deleted the add-another-settings-schema-version-due-to-changes-in-how-ios-403 branch December 4, 2023 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants