Conversation
|
🚨 End to end tests failed. Please check the failed workflow run. |
mojganii
left a comment
There was a problem hiding this comment.
@mojganii made 1 comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on pinkisemils).
ios/MullvadSettings/TunnelSettingsV7.swift line 79 at r1 (raw file):
if let settings = try? container.decode(IncludeAllNetworksSettings.self, forKey: .includeAllNetworks) { self.includeAllNetworks = settings } else {
Nicely done, but it should have been handled in upgradeToNextVersion() in TunnelSettingsV6. We shouldn’t have removed keys from TunnelSettingsUpdate, the old ones should be marked as deprecated. I suggest adding a comment to it to draw attention to this.
can you please add something like it above TunnelSettingsUpdate:
Code snippet:
// Note:
// Existing keys in `TunnelSettingsUpdate` must not be removed.
// They are required for backward compatibility.
// If a key is no longer used, mark it as deprecated instead of deleting it.
// Version upgrades should be handled in `upgradeToNextVersion()`.
pinkisemils
left a comment
There was a problem hiding this comment.
@pinkisemils made 1 comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on mojganii).
ios/MullvadSettings/TunnelSettingsV7.swift line 79 at r1 (raw file):
Previously, mojganii wrote…
Nicely done, but it should have been handled in
upgradeToNextVersion()inTunnelSettingsV6. We shouldn’t have removed keys fromTunnelSettingsUpdate, the old ones should be marked as deprecated. I suggest adding a comment to it to draw attention to this.
can you please add something like it above TunnelSettingsUpdate:
Is 2025.10 running V6? Nope, 2025.10 already has V7. We decided to have this lightweight migration because there have been no production apps that actually use these fields - it has only been debug fields.
rablador
left a comment
There was a problem hiding this comment.
@rablador reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on mojganii and pinkisemils).
ios/MullvadSettings/TunnelSettingsV7.swift line 81 at r1 (raw file):
} else { let includeAllNetworks = try container.decodeIfPresent(Bool.self, forKey: .includeAllNetworks) ?? false let legacyContainer = try decoder.container(keyedBy: LegacyCodingKeys.self)
If an older version of local network sharing exists, it should be enough to only decode it (if present) directly with Bool as type. No need to create a container, right?
mojganii
left a comment
There was a problem hiding this comment.
@mojganii made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on pinkisemils).
ios/MullvadSettings/TunnelSettingsV7.swift line 79 at r1 (raw file):
Previously, pinkisemils (Emīls Piņķis) wrote…
Is 2025.10 running V6? Nope, 2025.10 already has V7. We decided to have this lightweight migration because there have been no production apps that actually use these fields - it has only been debug fields.
sure, but that comment wouldn't hurt to be there
mojganii
left a comment
There was a problem hiding this comment.
@mojganii made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on pinkisemils).
ios/MullvadSettings/TunnelSettingsV7.swift line 79 at r1 (raw file):
Previously, mojganii wrote…
sure, but that comment wouldn't hurt to be there
we hid that behind the Debug flag then it means it's replicated on release branches anyway. it needs an extra care for the future.
d7360e5 to
bed14bb
Compare
pinkisemils
left a comment
There was a problem hiding this comment.
@pinkisemils made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on mojganii and rablador).
ios/MullvadSettings/TunnelSettingsV7.swift line 79 at r1 (raw file):
Previously, mojganii wrote…
we hid that behind the Debug flag then it means it's replicated on release branches anyway. it needs an extra care for the future.
It wasn't hidden behind a debug flag in the past, so it the fields exist in production keychains today. We are patching it over here during deserialisation. This could be turned into a new settings migration too, but we chose not to do that.
ios/MullvadSettings/TunnelSettingsV7.swift line 81 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
If an older version of local network sharing exists, it should be enough to only decode it (if present) directly with Bool as type. No need to create a container, right?
I've simplified it now, I don't think it matters to read the old values since no production app ever stored anything in there.
rablador
left a comment
There was a problem hiding this comment.
@rablador reviewed 2 files and all commit messages, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on mojganii).
mojganii
left a comment
There was a problem hiding this comment.
@mojganii made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on pinkisemils).
ios/MullvadSettings/TunnelSettingsV7.swift line 79 at r1 (raw file):
Previously, pinkisemils (Emīls Piņķis) wrote…
It wasn't hidden behind a debug flag in the past, so it the fields exist in production keychains today. We are patching it over here during deserialisation. This could be turned into a new settings migration too, but we chose not to do that.
I'm not arguing why we have it here. all I'm saying is if we had the above comment at "TunnelSettingsUpdate.swift" then we could catch it earlier. I still think we need this comment as a reminder for cleaning or updating a key
bed14bb to
703cbb3
Compare
pinkisemils
left a comment
There was a problem hiding this comment.
@pinkisemils made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on mojganii).
ios/MullvadSettings/TunnelSettingsV7.swift line 79 at r1 (raw file):
Previously, mojganii wrote…
I'm not arguing why we have it here. all I'm saying is if we had the above comment at "TunnelSettingsUpdate.swift" then we could catch it earlier. I still think we need this comment as a reminder for cleaning or updating a key
I see. I've added the comment verbatim. You are correct - the comment hopefully will help. But the test should catch issues like these as well.
mojganii
left a comment
There was a problem hiding this comment.
@mojganii made 1 comment and resolved 1 discussion.
Reviewable status: 2 of 3 files reviewed, all discussions resolved.
rablador
left a comment
There was a problem hiding this comment.
@rablador reviewed 1 file and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved.
rablador
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved.
|
🚨 End to end tests failed. Please check the failed workflow run. |
Fixing the settings deserialization issue on
main. We could also just do a stupid try-catch and initialize it to defualt values if anything fails.This change is