-
Notifications
You must be signed in to change notification settings - Fork 238
[CodePush] Add support for Xcode11 #751
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
[CodePush] Add support for Xcode11 #751
Conversation
jp-andre
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.
Is there any chance we could have some test cases with this? Testing that parsing works correctly on some sample file, etc...
| @shortName("c") | ||
| @hasArg | ||
| @longName("build-configuration-name") | ||
| public buildConfigurationName: string; |
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.
Is this a mandatory argument?
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 I just saw in another line that it defaults to Release. Why not here?
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.
Yes, you're right. I already fixed it. Thanks!
| } | ||
| out.text(`Using the target binary version value "${marketingVersion}" from "${resolvedPbxprojFile}".\n`); | ||
|
|
||
| return Promise.resolve(marketingVersion); |
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.
I think we can skip the Promise.resolve() here since the method is async already.
|
@jp-andre We'll schedule a feature to add missing tests for all of the release commands in the next weeks. Right now, there are no tests for most of the Does that make sense to you? |
Yes, considering the situation this is perfectly understandable. I'll work on a release. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Issue: Starting with Xcode 11 when changing the version of the application, it is saved into the
project.pbxprojinstead ofInfo.plist. And link to$MARKETING_VERSIONis put ininfo.plist. This does not allow appcenter-cli to make the release a new version, because it cannot get the current version fromInfo.plist. Moreover, customers can change the version for each build configuration (Release or Debug and etc.) individually.Solution: We implemented logic that allows getting the
$MARKETING_VERSIONfrom theproject.pbxprojfile. Appcenter-cli will take the default version ofReleasebuild configuration, But if the customer wants to create a release for another Build configuration, he can add an option flag with the Build configuration name to the react-release command.