-
Notifications
You must be signed in to change notification settings - Fork 2
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
docs(README): reduced need for horizontal scroll of example #1
base: main
Are you sure you want to change the base?
Conversation
# Port to check for emulator startup status, defaults to 9099 (Cloud Firestore). | ||
# Change this if you are using specific emulators or non-standard ports. | ||
# See https://firebase.google.com/docs/emulator-suite/install_and_configure#port_configuration. | ||
check-port: '9099' |
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.
Just noticed by reading action.yml
that this is not true.
The script does not use 9099 by default, it is 8080:
firebase-emulator-action/action.yaml
Line 33 in 8332066
default: '8080' # Firestore emulator default port |
# Port to check for emulator startup status, defaults to 9099 (Cloud Firestore). | |
# Change this if you are using specific emulators or non-standard ports. | |
# See https://firebase.google.com/docs/emulator-suite/install_and_configure#port_configuration. | |
check-port: '9099' | |
# Port to check for emulator startup status, defaults to 8080 (Cloud Firestore). | |
# Change this if you are using specific emulators or non-standard ports. | |
# See https://firebase.google.com/docs/emulator-suite/install_and_configure#port_configuration. | |
check-port: '8080' |
A better way to do this - now that I've looked - is to access the hub's emulators
endpoint and get the list of running emulators - I verified it is dynamic and an emulator only appears once it has actually started. You cannot rely on the count because some emulators start others (functions in particular also starts eventarc and tasks...).
But you can send that through jq and get a list of the emulators running.
curl --silent http://localhost:4400/emulators |jq 'keys[]'
"auth"
"database"
"eventarc"
"firestore"
"functions"
"hub"
"logging"
"tasks"
"ui"
So instead of checking firestore (which may or may not be in the list), you can use the input list, split it to the expected emulators, then in the check iterate over the expected list to make sure they are all there
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.
Just noticed by reading action.yml that this is not true.
The script does not use 9099 by default, it is 8080:
True! Thanks for catching this @mikehardy 👏
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.
@HassanBahati to reduce the need for configuration at all. Right now we propose in the example to check the firestore port but what if they are not running firestore? Then they need to change it, and some will forget, or get it wrong and it's not a terrible thing but there is a little bit of effort involved. If instead the whole "check-port" thing was dynamic by interrogating the hub for running emulators and making sure all the emulators listed in the action were up, there's no need for a specific "check-port" config at all.
Combined with a verification that at least one emulator was in the list of emulators to start, we provably know that all listed emulators are started, no check-port config needed
@@ -24,30 +24,32 @@ steps: | |||
uses: actions/setup-java@v4 | |||
with: | |||
distribution: 'temurin' | |||
java-version: '17' | |||
java-version: 17 | |||
|
|||
# Install the Firebase Emulator Suite | |||
- name: Start Firebase Emulator Suite | |||
uses: firebase-emulator-action@v1 |
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.
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.
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.
@Ehesp any thoughts on this? Would it be best to tag newer releases with the ref
as the full version version v1.0.0
resulting into invertase/[email protected]
or to shortening it to maybe v1
resulting into invertase/firebase-emulator-action@v1
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.
In my experience publishing actions you must have the author yes, but you do not want to specify the full version because then people can't get semver-compatible bug fixes
You want people to use v1, and you want to have a v1 tag plus the vX.Y.Z releases, but every time you do a release you "slide" the v1 tag up to the new release so that people get the new release without effort. That's how everyone does it AFAIK
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.
Thank you @mikehardy for the insight.
I truly agree thats the same developer experience i have always had using actions where just v1 is defined rather than the full latest version tag.
Just to be sure, is this the point when we need to add a .github/workflows/release.yaml
or .github/workflows/release-tag.yaml
file to achieve what you have described so as to have vX
pointing to the full version's (eg vX.Y.Z
) as per the latest release?
Or there is a different way to achieve it ?
Tried releasing an action and when i made patches, reusing the same vX
tag for subsequent published patches was rejected as the tags needed to be unique.
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.
@mikehardy I have opened a PR #6 . Kindly have a look at it.
# Port to check for emulator startup status, defaults to 9099 (Cloud Firestore). | ||
# Change this if you are using specific emulators or non-standard ports. | ||
# See https://firebase.google.com/docs/emulator-suite/install_and_configure#port_configuration. | ||
check-port: '9099' |
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.
Just noticed by reading action.yml that this is not true.
The script does not use 9099 by default, it is 8080:
True! Thanks for catching this @mikehardy 👏
@@ -24,30 +24,32 @@ steps: | |||
uses: actions/setup-java@v4 |
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.
uses: actions/setup-java@v4 |
I also think, for a usage guide. We don't need this block that setups Java as that is an environment thats set up by the action.
Here is an example of an example usage of the action.
https://github.com/HassanBahati/firebase-emulator-action-example
name: Firebase Emulator Action Example
on:
push:
branches:
- main
jobs:
run-emulator:
runs-on: ubuntu-latest
steps:
# Checkout the repository
- name: Checkout Code
uses: actions/checkout@v4
# Start Firebase Emulator Suite
- name: Start Firebase Emulator Suite
uses: invertase/[email protected]
with:
firebase-tools-version: "latest"
emulators: "auth,firestore,functions,storage,database"
project-id: "test-project"
max-retries: "3"
max-checks: "60"
wait-time: "1"
check-port: "8080"
When checking the README I noticed that there was a horizontal scroll for not much benefit