-
Notifications
You must be signed in to change notification settings - Fork 373
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
firebase-app-distribution 1.0.0 #2151
Conversation
- deploy | ||
toolkit: | ||
bash: | ||
entry_file: step.sh |
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.
Script uses npm to install dependency which is not recommended:
https://github.com/bitrise-io/bitrise/blob/master/_docs/step-development-guideline.md#do-not-use-submodules-or-require-any-other-resource-downloaded-on-demand
homebrew formula seems to be available: https://formulae.brew.sh/formula/firebase-cli
on Linux stacks it may become preinstalled: bitrise-io/android#256
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.
This is not official package from Firebase.
As following official way, I use cURL instead.
https://firebase.google.com/docs/cli#install-cli-mac-linux
entry_file: step.sh | ||
deps: | ||
brew: | ||
- name: git |
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.
None of those dependencies seem to be used by script at entry file. Are they really needed?
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.
Not needed.
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.
instead specified curl for download firebase.tool
description: | | ||
Required. Your firebase token for CLI. https://firebase.google.com/docs/cli#cli-ci-systems | ||
is_required: true | ||
title: Firebase Token |
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.
This input should be probably sensitive.
token: $FIREBASE_TOKEN | ||
- ipa_path: $BITRISE_IPA_PATH | ||
opts: | ||
is_required: true |
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.
What about APKs? project_type_tags
list also Android.
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.
APK is also available with same interface. Please let me know it should be.
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.
Then input should be called APP_FILE_PATH
or something like that. And description should contain info that it may point to either APK or IPA.
Hi @d-date, I’m from Bitrise. Apologies for jumping in late. We’ve been working with another user on a community step (#2152) for some time earlier and was planning to release it on Monday. Although your contribution is much appreciated, we decided to pull in the other step for the following reasons:
Sorry for the awkward situation. |
Hi @koral--, Thanks for your efforts in the review of the PR. Sorry for the inconvenience. :( |
New Pull Request Checklist
Please mark the points which you did / accept.
bitrise run test
(in the step's repository)bitrise run audit-this-step
(in the step's repository - note, if you don't have this workflow in yourbitrise.yml
, you can copy it from the step template.)