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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+11 −9
Open

docs(README): reduced need for horizontal scroll of example #1

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,30 +24,32 @@ steps:
uses: actions/setup-java@v4
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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

firebase-emulator.yaml

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"

with:
distribution: 'temurin'
java-version: '17'
java-version: 17

# Install the Firebase Emulator Suite
- name: Start Firebase Emulator Suite
uses: firebase-emulator-action@v1
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
uses: firebase-emulator-action@v1
uses: invertase/firebase-emulator-action@v1.0.0

I think we as well hadn't prefixed the author to the action path, so this would result in errors such as this;
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Also the ref version currently has got to be the full path. I think this is what was set as the tag when publishing the current release.
image

so current it requires v1.0.0 and if just v1 is used, it errors as below;
image

Copy link
Contributor

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

Copy link
Author

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

Copy link
Contributor

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.

Copy link
Contributor

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.

with:
# The version of the Firebase CLI to install, (`npm install -g firebase-tools@${firebase-tools-version}`)
# Firebase CLI version to install, (`npm install -g firebase-tools@${firebase-tools-version}`)
firebase-tools-version: 'latest'

# A comma separated list of emulators to start.
# Comma separated list of emulators to start. This example is the default:
emulators: 'auth,firestore,functions,storage,database'

# The project ID to use, defaults to 'test-project'.
# Project ID to use, defaults to 'test-project'.
project-id: 'test-project'

# The maximum number of retries to attempt before failing the action, defaults to 3.
# Maximum attempted retries to before action fails, defaults to 3.
max-retries: '3'

# The maximum number of checks to perform before failing the retry, defaults to 60.
# Maximum checks to perform before retry fails, defaults to 60.
max-checks: '60'

# The time to wait between checks in seconds, defaults to 1.
# Seconds to wait between checks, defaults to 1.
wait-time: '1'

# The port to check for the emulators, defaults to 9099 (Cloud Firestore). Change this if you are using specific emulators. See https://firebase.google.com/docs/emulator-suite/install_and_configure#port_configuration.
# 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'
Comment on lines +51 to 54
Copy link
Author

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

Suggested change
# 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

Copy link
Contributor

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 👏

Copy link
Author

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

```
```
Loading