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

[eas-cli] integrate push key setup in ios builds #473

Merged
merged 5 commits into from
Jun 28, 2021
Merged

Conversation

quinlanj
Copy link
Member

@quinlanj quinlanj commented Jun 24, 2021

Checklist

  • I've added an entry to CHANGELOG.md if necessary.
  • I've tagged the changelog entry with [EAS BUILD API] if it's a part of a breaking change to EAS Build API (only possible when updating @expo/eas-build-job package).

Why

Bring eas-cli to parity with expo-cli. In expo-cli, we offer to setup push notifications for the user if it hasn't been setup already. We do the same in eas-cli, except if the user has specified they want to use credentials from credentials.json. Users who opt to use credentials.json do so because they don't want to store their credentials indefinitely on www. However, setting up a push key requires you to actually store your push key indefinitely on www. So if a user opts to use credentials.json, we skip push key setup altogether.

prereqs

Test Plan

  • manually tested

@quinlanj quinlanj requested review from dsokal and wkozyra95 June 24, 2021 02:11
@github-actions
Copy link

github-actions bot commented Jun 24, 2021

Size Change: +3.44 kB (0%)

Total Size: 37.6 MB

Filename Size Change
./packages/eas-cli/dist/eas-linux-x64.tar.gz 37.6 MB +3.44 kB (0%)

compressed-size-action

Copy link
Contributor

@wkozyra95 wkozyra95 left a comment

Choose a reason for hiding this comment

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

In expo-cli people have more issues with notifications on android, because on ios it's integrated with a build process.
Current implementation unifies those 2 workflows, but the user must know what they are doing. I'm not against adding that as an optional step to the build process, but I believe that it makes sense only if we are doing it for both platforms.

Also, I don't see a solution for that, but currently, if someone does not want to setup their push notification credentials they will get a prompt every time they build (unless they use non-interactive), it might be anoying

packages/eas-cli/src/build/ios/credentials.ts Outdated Show resolved Hide resolved
packages/eas-cli/src/build/ios/credentials.ts Outdated Show resolved Hide resolved
@@ -44,6 +51,57 @@ export async function ensureIosCredentialsAsync(
};
}

export async function setupPushKeyAsync(
Copy link
Contributor

Choose a reason for hiding this comment

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

consider moving

  • this logic to separate Action
  • calling this function from Ios credentials provider

Copy link
Member Author

@quinlanj quinlanj Jun 25, 2021

Choose a reason for hiding this comment

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

I moved some of this logic to a separate Action. There's some parts that are specific to the build command (abort if we are building for simulator, create credentials ctx from build ctx, etc) that I left behind. Let me know if this is consistent with what you meant, or if you'd rather just get rid of the whole method and refactor it elsewhere.

Copy link
Contributor

@wkozyra95 wkozyra95 Jun 28, 2021

Choose a reason for hiding this comment

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

you could move

This way:

It does not matter that much, so if you prefer to keep the current implementation I'm fine with it.

@dsokal
Copy link
Contributor

dsokal commented Jun 24, 2021

Looks good to me. I agree with @wkozyra95 that we should also add support for setting up Android push notification keys.

Users who opt to use credentials.json do so because they don't want to store their credentials indefinitely on www. However, setting up a push key requires you to actually store your push key indefinitely on www.

IMO EAS CLI supports credentials.json not only for people who don't want to store their credentials in our database. It could be useful for building a project on a CI. This way you have control over which exact credentials will be used for the build.
Having said that, I think we could consider adding push credentials key configuration to credentials.json. The key shouldn't be required but if it was defined, we could set it up on our servers. I'm totally aware that the push key doesn't have anything to do with the build process but it'd make the credentials setup process more unified. E.g. if a user already has credentials generated before using EAS Build they could "import" them to our servers using a single file.
Whatever you decide, I think it doesn't need to be implemented in this PR.

@wkozyra95
Copy link
Contributor

I think we could consider adding push credentials key configuration to credentials.json. The key shouldn't be required but if it was defined, we could set it up on our servers. I'm totally aware that the push key doesn't have anything to do with the build process but it'd make the credentials setup process more unified. E.g. if a user already has credentials generated before using EAS Build they could "import" them to our servers using a single file.
Whatever you decide, I think it doesn't need to be implemented in this PR.

I disagree with that, I don't think credentials.json should support that

  • it's easy to break production app this way (if we would update push key during build)
  • there is no use case when someone would benefit from that
    • for users that don't want to store credentials on our server we shouldn't suggest that
    • for ci builds non-interactive will be set either way
    • I don't have any idea why someone would use credentials.json in any other case than those 2 above, but if there is someone like that they can use credentials manager
  • build credentials in credntials.json can be used by the normal build process, push key would only exist there to be uploaded
  • it's not really obvious at what step it would be updated on server (during every build, during the interactive build, some special command in credentials manager)

The middle ground that might be more reasonable would be to display the same interactive setup process for the push key even if someone is using credentials.json. I'm not against that approach, but I also don't think there is a need for that.

@dsokal
Copy link
Contributor

dsokal commented Jun 24, 2021

Well, generally, I'm not saying that I want to add push credentials to credentials.json. I said we can consider that.
I think the biggest disadvantage of this idea is that it could break production apps by accident (if someone built a project with a different key than the one that's on our servers). A solution to this problem could be:

  • if the key is not yet set up - upload it to EAS servers.
  • check if the key in credentials.json is the same as in the db, and if it's not:
    • non-interactive mode: fail
    • interactive mode: ask the user if they want to replace it

The middle ground that might be more reasonable would be to display the same interactive setup process for the push key even if someone is using credentials.json. I'm not against that approach, but I also don't think there is a need for that.

I like this idea.

@quinlanj
Copy link
Member Author

On supporting push key setup for Android: It is easier to set this up for the user in iOS because we can do it programmatically. In Android, it is more troublesome for the user because we only support FCM key uploads and I suspect this is why we don't integrate it into the Android build flow currently. Users need to go into their Firebase console, get the key and paste it into the cli at build time which can be annoying. We may need to change the experience so users can 'give up' if they rather do it later, or look into programmatically setting up fcm for folks.

On someone not wanting to setup push notifications: Yeah, it will be annoying to keep prompting users in this case. A similar use case came up when we were wanting to integrate App specific passwords into the Submissions flow with bartek k, where we want to support a 'No, don't ask me again' option. This seems to be a recurring ask, I can look into exposing a stateful question (similar to the way we remember local passwords for user's Apple accounts) if we think it would be useful.

On credentials.json support: I like the middleground idea, so I've changed setup to happen regardless of whether someone is using credentials.json. I agree with dominik that it'd be nice to unify credentials in one place for the people who want to use credentials.json. But there are enough downsides that make this unattractive. As wojtek mentioned, they are meaningfully different from build credentials in terms of the way they are stored because we need to persist them in www in order for push notifs to work, and its not really obvious at what step it would be updated on www.

@quinlanj quinlanj requested a review from wkozyra95 June 25, 2021 23:47
@@ -44,6 +51,57 @@ export async function ensureIosCredentialsAsync(
};
}

export async function setupPushKeyAsync(
Copy link
Contributor

@wkozyra95 wkozyra95 Jun 28, 2021

Choose a reason for hiding this comment

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

you could move

This way:

It does not matter that much, so if you prefer to keep the current implementation I'm fine with it.

packages/eas-cli/src/build/ios/build.ts Outdated Show resolved Hide resolved
@quinlanj quinlanj merged commit 86e5e1a into main Jun 28, 2021
@quinlanj quinlanj deleted the @quin/pushKeySetup branch June 28, 2021 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants