-
Notifications
You must be signed in to change notification settings - Fork 78
[Dev] Support loading Emission from AppHub #541
Conversation
Going to look at this after our current release woes are over. |
Yah, that's fine, less changing it 👍 |
poke |
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.
Also needs a rebase.
@@ -103,7 +103,7 @@ - (void)setupEmissionWithUserID:(NSString *)userID accessToken:(NSString *)acces | |||
useStagingEnvironment:useStaging | |||
sentryDSN:nil]; | |||
#else | |||
#ifdef DEPLOY | |||
#if DEPLOY |
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.
Why do you use ifdef
in the file above but prefer if
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.
I might switch the app entirely to just debug and release, I don't think a third schema is useful as release will not ever be used
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 use release sometimes to test on a device.
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.
Alright - I'll keep it around 👍
if (diff == 1) return @"yesterday"; | ||
if (diff == 7) return @"last week"; | ||
|
||
return[NSString stringWithFormat:@"%@ days ago", @(diff)]; |
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.
Nit: missing whitespace
@@ -24,7 +24,7 @@ target 'Emission' do | |||
pod 'Artsy+UIFonts' | |||
|
|||
# For easy updating of the JS | |||
pod 'AppHub' | |||
pod 'AppHub', :git => 'https://github.com/orta/apphub.git', :branch => "build_list" |
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.
Good fix 👍
# | ||
# pod 'Specta' | ||
# pod 'Expecta' | ||
# end |
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 👍
scripts/deploy_apphub.sh
Outdated
# Pull the name of the PR out of the auto-generated commit description | ||
PR_DESC=`git log --format=%B -n 1 $SHA | tail -1` | ||
# This'll break when we hit 1k PRs, but that's still a while away and an easy fix | ||
PR_NUM=`git log --format=%B -n 1 $SHA | grep -o '#[0-9][0-9][0-9]' | tail -n 1` |
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 a reason you can’t use grep -Eo '#[0-9]+'
instead?
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.
If that works - even better :D
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.
Yup, the -E
is for BSD grep to support extended regexp, I believe that even GNU grep secretly supports the -E
option to be compatible with BSD grep.
Updated and addressed all feedback |
Ace 🙏🏽 |
Adds a new section when in the deploy schema (e.g. you will not see this in dev)
It has the same build chooser that Eigen has:
And can pull out the related PR from the build:
This makes the apphub upload also include the PR metadata as a description, so there should be more builds that look like:
I disabled the tests part of our Pods setup. Until we're writing native tests it adds more complications to the build process for everyone - and I was having some tricky issues with the React subspecs and AppHub.
Tested on Device?
How to get set up with this PR?
To run on your computer:
Then run
xcrun simctl launch booted net.artsy.Emission
once a the simulator has finished bootingTo run inside Eigen (prod or beta) or Emission (beta): Shake the phone to get the Admin menu.
If you see "Use Staging React Env" - click that and restart, then follow the next step.
Click on "Choose an RN build" - then pick the one that says: "X,Y,Z"
Note: this is a TODO for PRs, currently you can only do it on master commits.