percy / cli

The Percy CLI is used to interact with, and upload snapshots to, percy.io via the command line.
https://docs.percy.io/docs/cli-overview
71 stars 45 forks source link

Add basic parallel nonce support for GitHub Actions #296

Closed nicholaschiang closed 3 years ago

nicholaschiang commented 3 years ago

Right now, there doesn't seem to be support for GitHub Actions without having to configure things ourselves.

Right now, this is my current configuration (might be helpful when creating built-in support):

env:
  NODE_ENV: test
  PERCY_PARALLEL_NONCE: ${{ github.run_id }}
  PERCY_PARALLEL_TOTAL: 10
wwilsman commented 3 years ago

Hey @nicholaschiang!

We do actually support GH Actions, just not an automatic parallel nonce. The main requirement of the nonce is to be unique for a Percy build. In your case run_id will be the same for re-runs, which could cause errors if you ever need to re-run a workflow that failed half way. Though I do think even if this nonce causes an error on retries, it still might be better to have minimal support rather than no support at all.

However, percy-js will be deprecated soon in favor of @percy/env, so I'm going to move this issue over there for tracking. 👍

wwilsman commented 3 years ago

For completeness, if you do run into issues with retries you'll have to create a more unique nonce at the start of the workflow, and then persist it throughout your steps/jobs. For example:

# ...

env:
  PERCY_PARALLEL_TOTAL: 10

jobs:
  # run this job first
  nonce:
    runs-on: ubuntu-latest
    # persist job results to other jobs in the workflow
    outputs:
      result: ${{ steps.nonce.outputs.result }}
    steps:
    # persist step results to other steps in the job
    - id: nonce
      # adding a timestamp makes the nonce more unique for re-runs
      run: echo "::set-output name=result::${{ github.run_id }}-$(date +%s)"

  # other jobs then rely on the first job with `needs`
  run-tests:
    matrix: # ...
    runs-on: ${{ matrix.os }}
    needs: nonce
    env: 
      PERCY_PARALLEL_NONCE: ${{ needs.nonce.outputs.result }}
    steps:
    - # ...

(see output parameters & jobs.<job_id>.outputs)

Robdel12 commented 3 years ago

This makes me wonder if we want to adopt an automatic nonce for actions -- I don't think so. I think our docs should outline that you'll have to create a (reliable & proper) nonce like your example @wwilsman.

We have this same issue with TravisCI and it did troll a lot of folks with their parallel builds (they reuse the same ID on rebuilds -- breaks Percy).

wwilsman commented 3 years ago

@Robdel12 do you think some minimal support, even with a retry error, is better than no automated support at all? And if not, should we then remove any other automated nonce that fails this requirement?

Like you said, this is currently an issue with Travis. So do we let GH actions have the same basic support with limitations, or do we remove the limited, semi-broken, support from Travis (and audit the other nonces)?

I do think we should be consistent one way or the other; just a matter of which way.

ismay commented 3 years ago

I can imagine that github will come up with an env var soon that will be unique for each rerun. Their current supply of env vars doesn't have anything like that, and it's causing problems for other libs as well (parallelized cypress runs for example). The workaround for cypress is much the same as what you've proposed here @wwilsman.

Once a unique build id is supplied by GH we could use that as the nonce. Until then I don't think I would handle this on Percy's side too much. Imo github should address this. Though that doesn't solve the immediate issue of us having to use this workaround, but still.

edit: actually, after looking more closely at the cypress action docs, I see that they accept a GITHUB_TOKEN. If you pass that, they'll take care of generating a unique id for you. That seems like a nice solution: https://github.com/cypress-io/github-action#parallel

wwilsman commented 3 years ago

Now that we have basic nonce support with #375, we can probably close this. It will always be archived for people coming across the uniqueness issue with re-runs, plus I believe we plan to add a doc soon that describes what to do when running across the issue.

@Robdel12 do you agree this is probably closable?

Robdel12 commented 3 years ago

Gooooooood question. I was thinking this stays open but gets repurposed to be closed when we have the new parallel docs up with an advanced section that highlights the shortcomings of the current automatic nonce setting.

Robdel12 commented 3 years ago

I added a new section to the docs page along with a CTA for other examples if folks would like to contribute: https://docs.percy.io/docs/parallel-test-suites#advanced-nonce-setting