search

eduvpn / apple

app for iOS and macOS
Other
62 stars 18 forks source link

Initial version of Let’s Connect! with redesign #326

Closed roop closed 4 years ago

roop commented 4 years ago

This version of Let’s Connect! includes discovery. Discovery shall be removed later.

Key thing to review here is data migration. Migration is done only by looking at the authState.bin and client.certificate locations.

roop commented 4 years ago

As a safeguard (i.e. just in case there's a problem with my migration code), the old data is not removed. It can be removed in a later release.

roop commented 4 years ago

Looks good to me. Maybe add a bit of error handling. Now there are several cases where failure is silenced. Do we want that? If so, approved.

@jeroenleenarts If error handling is lacking, that's a bug. If you're seeing that errors that should be reported are being silently ignored, please point them out so I can address them.

jeroenleenarts commented 4 years ago

Several nil checks short circuit processing. Either with an early empty value return or a continue statement.

Those are all cases with valid fail cases which might require retry later instead of silent ignore and somewhat loss of data (by not migrating). Yes the original data is not destroyed, but if migration fails due to a random reason I assume it will never be migrated.

roop commented 4 years ago

AFAIK, all those cases indicate there's no usable data to migrate. Maybe you can add a code comment to wherever you think we should be handling it differently?

roop commented 4 years ago

Had to force push to fix a typo in a commit message ("Migrate date from ..." -> "Migrate data from ...").

roop commented 4 years ago

AFAIK, all those cases indicate there's no usable data to migrate. Maybe you can add a code comment to wherever you think we should be handling it differently?

@jeroenleenarts I'm going to merge it in now, but If you think there's an issue here, please still add a code comment here and I'll fix that in a later PR.