nodejs-docs-samples icon indicating copy to clipboard operation
nodejs-docs-samples copied to clipboard

testing: Add support for runtime environment variables to GitHub Workflows

Open grayside opened this issue 2 years ago • 3 comments

GitHub Workflows is the preferred test driver for tests in this repository, but the current workflow implementation has several limitations. One of them is the ability to set runtime environment variables that are specific to a Workflow.

Approach

Options:

  1. Hard code a central map of environment variables, and refactor code to ensure clean namespacing
  2. Create a mechanism to pass environment variables to our reusable test workflow

Option 2 is the most versatile with the least need to refactor existing code.

Option 1 has the further downside of placing test-specific parameters in a central file. I'm not sure if any changes to test.yml are triggering wide-scale test execution currently, but with this approach we'd likely need to trigger more tests from any change, which would include adding environment variables for unrelated tests.

@pattishin has asked that as part of the PR fixing this issue, we look for existing hard-coded values and shift them to environment variable configuration.

This issue is a blocker on implementation of GHA-based test workflows for a number of samples, with #3149 as the driver for research today.

grayside avatar Sep 12 '23 21:09 grayside

Here are some notes towards Option 2 from the OP:

Illustrating the thought just a bit further (this is a thought experiment, not tested!)

in functions-v2-imagemagick.yaml:

Add a new job:

  setup:
    runs-on: ubuntu-latest
    permissions: {}
    steps:
    - run: |
        echo "FUNCTIONS_BUCKET=nodejs-docs-samples-tests" >> $GITHUB_OUTPUT
        echo "BLURRED_BUCKET_NAME=nodejs-docs-samples-tests-imagick" >> $GITHUB_OUTPUT
        cat $GITHUB_OUTPUT

Add use of this to the test job:

  test:
    # Ref: https://github.com/google-github-actions/auth#usage
    permissions:
      contents: 'read'
      id-token: 'write'
    if: github.event.action != 'labeled' || github.event.label.name == 'actions:force-run'
    uses: ./.github/workflows/test.yaml
    with:
      name: 'functions-v2-imagemagick'
      path: 'functions/v2/imagemagick'
      env: needs.setup.outputs
    needs: [setup]

In test.yaml add a new input:

      env:
        type: string

Then after the authentication step, load the environment from input to env:

    - name: Set caller env
      run: |
        echo "${{inputs.env}}" >> $GITHUB_ENV

If some variant of this works, we now have a way for the calling workflow to pass unexpected environment variables to the test workflow.

grayside avatar Sep 12 '23 21:09 grayside

Just FYI, we may not be blocked, as this capability already exists in the workflow generator, with several samples using this workflow. This ticket could cover potential refactorings if needed.

kweinmeister avatar Sep 13 '23 00:09 kweinmeister

Ah, thank you for pointing this out. I saw a couple examples of the samples using the workflow-secrets template but I interpreted them as one-off forks of the reusable test.yml.

I don't have access to review the existing secrets, but there is notionally a limit of 100 per repo: https://docs.github.com/en/actions/security-guides/using-secrets-in-github-actions#limits-for-secrets.

This opens three tracks for this issue:

  1. Close the issue, we have the capability and are not concerned with the limitation
  2. Refactor how we use secrets because the limitation is a concern (e.g., a single secret per product that holds a set of values)
  3. Switch to something like the model in this issue (if it works) and limit secrets to configurations with security concerns

The advantage of the approach proposed in this issue is it helps reduce structural variation in the individual workflows, meaning we may have more flexibility in the future to group tests or apply matrix strategies.

I'm happy to move forward with #3149 via the workflow-secrets template in the meantime.

grayside avatar Sep 14 '23 16:09 grayside