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

(wip - don't merge) : build: codepush sdk #9137

Closed
wants to merge 42 commits into from
Closed

Conversation

brainbicycle
Copy link
Contributor

@brainbicycle brainbicycle commented Aug 8, 2023

This PR resolves PHIRE-156

Description

Integrates the code push SDK on iOS and Android, for the time being inactive until we get the build and ci scripts in place.

Exposed the staging deploy key in the process, that was rotated.

PR Checklist

  • I have tested my changes on iOS and Android.
  • I hid my changes behind a feature flag, or they don't need one.
  • I have included screenshots or videos, or I have not changed the UI.
  • I have added tests, or my changes don't require any.
  • I added an app state migration, or my changes do not require one.
  • I have documented any follow-up work that this PR will require, or it does not require any.
  • I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

  • I would like at least one of the reviewers to run this PR on the simulator or device.
Changelog updates

Changelog updates

Cross-platform user-facing changes

iOS user-facing changes

Android user-facing changes

Dev changes

  • Integrates codepush SDK - Brian, George

Need help with something? Have a look at our docs, or get in touch with us.

@@ -36,7 +36,7 @@
<key>CFBundlePackageType</key>
<string>APPL</string>
<key>CFBundleShortVersionString</key>
<string>7.3.6</string>
<string>8.18.0</string>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

need to start updating the version number in the project so codepush, codepush reads from the plist, can potentially override but think we can update the script we run in release process to update everywhere

@ArtsyOpenSource
Copy link
Contributor

ArtsyOpenSource commented Aug 8, 2023

This PR contains the following changes:

  • Dev changes (Integrates codepush SDK - Brian, George)

Generated by 🚫 dangerJS against f8535c8

deploymentKey: Config.CODE_PUSH_STAGING_DEPLOYMENT_KEY,
checkFrequency: codePush.CheckFrequency.MANUAL,
}
: { deploymentKey: Config.CODE_PUSH_PRODUCTION_DEPLOYMENT_KEY }
Copy link
Member

@damassi damassi Aug 8, 2023

Choose a reason for hiding this comment

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

(blocking) Can we move this system-y setup code into app/system/codepush.ts so that it is discoverable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean just the options? sure, can do that

Copy link
Member

Choose a reason for hiding this comment

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

Yep, this block that configures codepush; we can import it from system and apply it to App here in the root

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 will do

@brainbicycle brainbicycle marked this pull request as ready for review August 15, 2023 17:49
@brainbicycle brainbicycle changed the title build: codepush sdk (wip - don't merge) : build: codepush sdk Aug 15, 2023
calculated_hash=$("$DIR/calculate-native-hash.sh")

# Retrieve the current native code version from app.json
native_code_version=$(jq -r '.nativeCodeVersion | to_entries | reduce .[] as $item (0; . as $key | if $item.key | tonumber > $key then $item.key | tonumber else $key end)' app.json)
Copy link
Member

Choose a reason for hiding this comment

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

That's cool -- wanted to use jq in the scripts, so just installed an orb 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah was thinking I might need to avoid it but doesn't seem so bad, the scripts are all still a bit in flux as I do some more exploration of what ci should look like but doesn't seem too bad

@brainbicycle brainbicycle requested a review from a team August 24, 2023 13:57
Copy link
Member

@gkartalis gkartalis left a comment

Choose a reason for hiding this comment

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

EXCITING

@brainbicycle
Copy link
Contributor Author

Going to split this up into a couple PRs since it is doing a few different things and getting large 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants