artsy / README

:wave: - The documentation for being an Artsy Engineer
Creative Commons Attribution 4.0 International
1.1k stars 120 forks source link

RFC: use codepush for PR QA and faster releases #468

Closed pvinis closed 2 years ago

pvinis commented 2 years ago

codepush

joeyAghion commented 2 years ago

This is an exciting opportunity. Reviewing PRs would be a nice-to-have, but the ability to ship hot-fixes or save days of release time (and so start to release more often) would be huge. I tend to agree that the slowness and irreversibility of the current process makes us very cautious.

Of course, the devil will be in the details. And adding another dimension (of which release and bundle someone's on) to the already confusing mix of beta/store, staging/production, simulator/device, and feature flag considerations could create even more confusion when it comes to tracing errors.

pvinis commented 2 years ago

the most confusing parts right now are not the actual releases, since they are just 2 scripts that work very easily. and codepush would add another script. that part is fine.

the harder part is the scheduling/QAing, which I'm trying to ease with this rfc. so my plan is to not complicate the release docs much more besides the js vs native code, and to uncomplicate by having an easy release/hotfix when we need it.

rajsam003 commented 2 years ago

Love it ❤️

mdole commented 2 years ago

whoa. this sounds like the future, TBH. would be huge if we can just release whenever we want and QA much more easily.

One thing we should get a sense for before committing: how much will it cost? In the pricing section, I see 240 build minutes per month and then $40 per month per "build concurrency" and $99 per month per "device concurrency"...I have no clue what that means. Do you understand it @pvinis ?

pvinis commented 2 years ago

@mdole for pricing, it would be free, as we don't need to build with them. my plan is to build in circle, and just deliver it to appcenter only for the OTA part.

I'll verify it's actually free and possible again, but that's how I've used it before.

admbtlr commented 2 years ago

I'm very excited about the DX possibilities of Codepush: being able to run arbitrary branches on a device by just selecting a bundle from a dropdown seems like a small miracle to me.

But I'm sceptical about going full CD, partly for the reason that I mentioned in my comment above. This is a good article about trying out Codepush with a complex app, and how in the end they decided not to use it:

https://www.matthewsessions.com/blog/code-push/

There are a lot of subtle payoffs that we would need to think about before committing to a new set of processes.

But I'm in favour of trying it! I just think we need to be realistic about what we're likely to gain from it.

pvinis commented 2 years ago

A little update here.

I want to make this RFC mostly about PR testing first.

I see some people are not too familiar with how things work on codepush, and I see that we definitely need to try out some things first until we are all comfortable with how the whole system works and what control we have over it. So I would like to slightly pivot this RFC for just the PR testing before merging first, and once we are done with that, we can evaluate how/if we want to move further with codepush and use it for hotfixes, and even later if/how we want to use it for releases.

kathytafel commented 2 years ago

So Mounir said Apple "may not like." we need to make sure that whatever we do is within the bounds of the developer agreement we sign, or there is the problem that we could be removed from the store.

2.5.2 Apps should be self-contained in their bundles, and may not read or write data outside the designated container area, nor may they download, install, or execute code which introduces or changes features or functionality of the app, including other apps. ...

Certainly we can't add features or change functionality.

pvinis commented 2 years ago

I don't know about that.

3.3.2 Except as set forth in the next paragraph, an Application may not download or install executable code. Interpreted code may only be used in an Application if all scripts, code and interpreters are packaged in the Application and not downloaded. The only exceptions to the foregoing are scripts and code downloaded and run by Apple's built-in WebKit framework or JavascriptCore, provided that such scripts and code do not change the primary purpose of the Application by providing features or functionality that are inconsistent with the intended and advertised purpose of the Application as submitted to the App Store.

it mentions not changing the "primary purpose" of the app, and the "intended and advertised purpose". Which in my mind we have not changed since I joined artsy. The app allows users to buy and sell art. 🤔

Though, as my previous comment says, let's try that for PRs first, and see how we like it later for releases etc maybe.

admbtlr commented 2 years ago

The only exceptions to the foregoing are scripts and code downloaded and run by Apple's built-in WebKit framework or JavascriptCore

This is the phrasing they added a while ago to explicitly allow code push; the "or JavascriptCore" part didn't used to be there (see for example this blog post that quotes an earlier version of this paragraph, without the JSC allowance) (remember when Meteor was the future?!). I wonder whether they'll change it soon, now that Hermes is the default runtime instead of JSC?

Anyway, there are plenty of examples of apps that use code push on the app store, so I don't think we should be too worried about that. But I'm still not convinced about the gains we would get from using it for updates, so I agree that we should start by trying it out for PR testing.

kathytafel commented 2 years ago

I think we should be looking at the spirit - which is that they want to review what's on the store, and they don't want you changing it after the fact so much. In addition, I think there's an aspect of user consent here. I consent to changing the software on my phone when I download things from the App Store. I personally don't want random bits of software installing itself without my awareness. Obviously for ease of use I don't want to be manually updating everything. However, we shouldn't think that we own the container on everyone's devices. The user owns their device not us. They should choose what goes on it.

For PRs though I think this is effing cool. We should all be able to test what colleagues are adding easily.

pvinis commented 2 years ago

I will change the text on the files of this PR to reflect what we are talking about here, and wait for next week before a final decision. 👍

(the important bit of the comment here is the thing above. feel free to skip the rest of the comment, it's slightly off-topic, but still relevant.) As an aside, and that can be a big discussion, I wanna say that yes, obviously the user owns their device, and decide what to put on it. And they always have the choice of removing something. When a user installs something it's either because they want to try it, which is a fine usecase, they can always delete right after, or it's because they trust it. I can't control what Apple put on my device, but by buying an iPhone I implicitly trust Apple with my mobile software and hardware needs. Same deal with apps. By installing google maps, I trust Google will give me the directions I need, knowing that they might use some of the tracking. For me it's a fair trade for that case. By installing WhatsApp or Signal, I trust that with every update they will still allow me and my friends to communicate mostly by text, as the apps were advertised. I trust they will not ban text messaging. They might add "stories" functionality, that I can decide to use, not use, keep or delete the app because of it. Same deal with the Artsy app. I, like any other user, trust that Artsy will keep improving the app in the way they think is best for me to buy and sell art. I trust they will not run weird code on my device. They might. They might not. But I trust they won't. And if they ever do, I always reserve the right to delete and 1-star them. And if they don't I would be a happy user 5-starring them all day long.

kathytafel commented 2 years ago

I know all that and it's a matter of when the updates happen - Apple asks me if I want to have it automatically. I don't put google maps on my device so they don't track me. Financial apps mostly put up a forced app update flag to download updates from the store. And to be clear having a client server approach where we can change the server after the fact is adding new functionality. It's just that the user should control when software is installed. Otherwise we are close to malware. Even if we will never ever in 100 million years do something janky with our software, the behavior looks similar. Nobody should trust anyone ever with software. I certainly don't trust WhatsApp or TikTok, for instance, and I long ago deinstalled the Facebook app. Maybe I overindex on threat modeling, but better safe than sorry, especially as we become more of a marketplace.

emmadickson commented 2 years ago

I'm on board with this!

brainbicycle commented 2 years ago

Thanks for writing this up! I am on board with trying out codepush for PRs + hot fixes, sounds great!

Release Process Comment: Outdated Realizing this is not the gist of the RFC any more but just calling out I am not on board with replacing our existing release process with code push.

There are anecdotal issues quoting App Store rejections explicitly calling out code push some of them after 3.3.2 was updated: https://github.com/microsoft/react-native-code-push/issues/1898, https://github.com/microsoft/react-native-code-push/issues/1297. And I agree with Kathy the spirit of the law is Apple wants to review your code when you add significant functionality.

The quoted guidelines interpretation is coming from Microsoft who owns codepush so it is important to take it with a grain of salt. It sounds from 3.3.2 that code push is allowed with the callout for JSCore, ...but it also sounds like it is contradicted by 2.5.2. How it is applied is up to Apple and I don't like the idea of banking our continued release of the app on that. Worth mentioning previous companies that provided hot code push functionality assured people that they were in compliance with App Store review guidelines, then one day all those apps started getting rejected: https://news.ycombinator.com/item?id=13817557 The mechanism was different and more risky (Obj-C runtime magic vs js updates) but I worry the risk remains without some specific assurances from Apple.

Anyways I know this was removed from RFC just my 2c for future reference.

pvinis commented 2 years ago

Resolution

We decided to do it as a PR List. Everyone agrees that using codepush as a way to test a PR with no native code before merging to main is the best gain for us.

Most people are not into using/replacing our existing release ways. Something to be looked at again in the future.

Level of Support

1: Overwhelming positive feedback. (for the PR List part.)

Next Steps

Exceptions

N/A

damassi commented 1 year ago

As mentioned in Slack, perhaps we can give this a try with Folio to get some experience and then reevaluate in terms of Eigen? A trial run.