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

ENG-5771: Add firebase app distribution support #2769

Merged
merged 6 commits into from
Nov 6, 2024

Conversation

seanirby
Copy link
Contributor

@seanirby seanirby commented Oct 30, 2024

Describe your changes

This PR updates the Fastlane plugin to optionally support a configuration for firebase app distribution. If the config is provided the plugin will add the necessary lanes. Besides including this configuration, the only other step for projects to integrate with FAD is to update their Github actions/workflows and to create a secret in their repo, GOOGLE_SERVICE_CREDENTIALS, which contains the firebase credentials for the google service account.

Overview of changes:

  • Update Fastfile templates to provide conditional firebase lanes.
  • Update Pluginfile templates to conditionally provide the necessary appcenter and/or firebase gems.
  • Fastlane Pluginfiles templates are now rendered to account for conditional gem installations
  • Updated tests

NOTE: I still need to verify Firebase Android release builds. I'm getting a failure on those builds because the app isn't linked to a Google Play account.

Firebase release uploads have been verified to be working with this plugin logic using the theory app.

Screenshot 2024-11-04 at 10 51 05 AM

https://console.firebase.google.com/project/theory-57e35/appdistribution/app/android:com.fastretailing.theory/releases

Issue ticket number and link

https://brandingbrand.atlassian.net/browse/ENG-5771

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Test Plan

I created an example shopify template using the flagship accelerator. I used the config provided below and updated my repo to include the google service credential secret for the shopify project. I've given Nick Burkhart view permissions to the project so he can check it out. Currently the firebase builds are working for both platforms except the release Android builds aren't uploading because there isn't a Google Play account linked to the Firebase Shopify app.

  codePluginFastlane: {
    plugin: {
      ios: {
        appCenter: {
          organization: 'Branding-Brand',
          appName: 'Shopify-iOS-Internal',
          destinationType: 'group',
          destinations: ['IAT'],
        },
        firebase: {
          groups: ['IAT'],
          appId: '1:300374277703:ios:cdf8a81375502895bcbb62',
        },
      },
      android: {
        appCenter: {
          organization: 'Branding-Brand',
          appName:
            'Shopify-Android-Internal',
          destinationType: 'group',
          destinations: ['IAT'],
        },
        firebase: {
          groups: ['IAT'],
          appId: '1:300374277703:android:764c235c8f6d7dc6bcbb62',
        },
      },
    },
  },

Checklist before requesting a review

  • A self-review of my code has been completed
  • Tests have been added / updated if required
  • Documentation has been updated to reflect these changes

@seanirby seanirby marked this pull request as ready for review November 4, 2024 18:52
@NickBurkhartBB
Copy link
Contributor

Note: as currently setup in PR this is a breaking change as it changes the lanes you use to upload.

@seanirby
Copy link
Contributor Author

seanirby commented Nov 5, 2024

I've addressed PR comments and also updated the lanes so this update should be nonbreaking. I did a quick test on theory repo with the existing workflows: https://github.com/brandingbrand/theory/actions/runs/11692375987

Copy link
Contributor

@NickBurkhartBB NickBurkhartBB left a comment

Choose a reason for hiding this comment

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

@seanirby It looks good, just a nitpick about the outputted Fastfile, the whitespace is a little messed up due to extra indents in the template due to indenting based off EJS if statements.

@@ -31,6 +31,37 @@ lane :increment_build do
end
<% } -%>

lane :increment_build_appcenter do
<% if(codePluginFastlane.plugin && codePluginFastlane.plugin.android && codePluginFastlane.plugin.android.appCenter) { -%>
increment_build
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: whitespace of output is off, remember the if statement is EJS and not part of the output so don't need to indent things below it or you get double indentation

@@ -76,6 +76,15 @@ lane :increment_build do
end
<% } -%>

lane :increment_build_appcenter do
<% if(codePluginFastlane.plugin && codePluginFastlane.plugin.ios && codePluginFastlane.plugin.ios.appCenter) { -%>
increment_build
Copy link
Contributor

Choose a reason for hiding this comment

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

whitespace of outputted Fastfile, no indent after EJS


lane :increment_build_firebase do
<% if(codePluginFastlane.plugin && codePluginFastlane.plugin.ios && codePluginFastlane.plugin.ios.firebase) { -%>
begin
Copy link
Contributor

Choose a reason for hiding this comment

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

whitespace in output, no indent after EJS if

build

<% if(codePluginFastlane.plugin && codePluginFastlane.plugin.ios && codePluginFastlane.plugin.ios.appCenter) { -%>
appcenter_upload(
Copy link
Contributor

Choose a reason for hiding this comment

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

whitespace of output, no indent after EJS if

@seanirby
Copy link
Contributor Author

seanirby commented Nov 6, 2024

@seanirby It looks good, just a nitpick about the outputted Fastfile, the whitespace is a little messed up due to extra indents in the template due to indenting based off EJS if statements.

I'll fix this up.

@NickBurkhartBB NickBurkhartBB merged commit f26b470 into brandingbrand:develop Nov 6, 2024
1 check passed
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.

2 participants