-
Notifications
You must be signed in to change notification settings - Fork 15
feat: adding public key bundling on iOS #55
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
Conversation
| yamlContent.writeln('auto_update: $autoUpdate'); | ||
| } | ||
|
|
||
| final String? shorebirdPublicKeyEnvVar = Platform.environment['SHOREBIRD_PUBLIC_KEY']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is probably an easier way to pass the key to ios since we edit the yaml in a dart context. But in order to keep it simple and consistent to android, I thought it would make since to do the same thing here too.
| ); | ||
| }); | ||
|
|
||
| // TODO(erickzanardo): Add tests for flavors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dynamically adding flavors to an iOS doesn't seems to be simple, so to not put too much time in it, for now I just tested the cases that there are no flavors. Happy to revisit this though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is normally set up through xcode. I'm sure there's some xcrun command we could run or scheme file we could copy, but agreed that it's probably not within the scope of this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep. I tried looking for a xrun command but I couldn't find any 🙃
There is an interesting flutter package called flutter_flavorizr which I might give it a try later. I just felt like not putting much more time in it so we can get to the demo soon!
But I plan on coming back here, since I feel these are important tests to make!
bryanoltman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, few small comments
| Map<String, String>? environment, | ||
| }) async { | ||
| final result = await _runFlutterCommand( | ||
| // We are using `--no-codesign` because we are not signing the app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment doesn't tell us much that we couldn't glean from the arg itself. It should probably answer questions like "why is the argument necessary" (i.e., why aren't we signing the app)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, looking at the comment now, I feel dumb for writing it haha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh no, that wasn't my intention at all! 😬 Writing good comments/docs is tough, and I do this kind of thing all the time too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah totally, and I agree, I hate comments that say what is clear to what is being done just by the method name hahaha! Thanks for calling it out!
Co-authored-by: Bryan Oltman <[email protected]>
Adds the bundling of the public key in the iOS binaries.