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

feat: make firebaseServiceAccount input optional #398

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

lucetre
Copy link

@lucetre lucetre commented Dec 6, 2024

Key Changes:

  1. Optional firebaseServiceAccount Input:
    The firebaseServiceAccount input is no longer required. If not provided, the action defaults to using the Application Default Credentials (ADC) available in the GitHub Actions environment. This reduces the need for explicitly managing and passing a service account key JSON file.
jobs:
  demos:
    runs-on: ubuntu-latest
    permissions:
      contents: "read"
      id-token: "write"

    steps:
      - uses: actions/checkout@v4
      - uses: google-github-actions/auth@v2
        with:
          project_id: ${{ vars.PROJECT_ID }}
          workload_identity_provider: ${{ vars.WORKLOAD_IDENTITY_FEDERATION_PROVIDER_ID }}
          service_account: ${{ vars.SERVICE_ACCOUNT }}

      - uses: FirebaseExtended/action-hosting-deploy@main
        with:
          # firebaseServiceAccount: CAN_BE_OMITTED
          projectId: ${{ vars.PROJECT_ID }}
          channelId: live
          entryPoint: ./demo

Expected Outcomes

  1. Improved Security:
    By leveraging default credentials, this change minimizes the usage of static key JSON files, which can pose security risks if mishandled or exposed.
  2. Simplified Workflow Configuration:
    Users can now avoid creating and storing a firebaseServiceAccount secret unless explicitly needed, streamlining the setup process and reducing overhead.
  3. Backward Compatibility:
    Existing workflows that supply a firebaseServiceAccount key JSON will continue to function as before, with no changes required.

bin/action.min.js Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/deploy.ts Outdated Show resolved Hide resolved
Copy link

@DevSusu DevSusu left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@lucetrez lucetrez left a comment

Choose a reason for hiding this comment

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

👍

Copy link

@cychoi1q cychoi1q left a comment

Choose a reason for hiding this comment

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

lgtm

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.

4 participants