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

Do not fail test run if recording without record key for pull requests from forked repos #1193

Closed
bahmutov opened this issue Jan 15, 2018 · 8 comments · Fixed by #1505
Closed
Assignees
Labels
cli type: enhancement Requested enhancement of existing feature type: user experience Improvements needed for UX
Milestone

Comments

@bahmutov
Copy link
Contributor

improvement

We currently stop tests if the user wants to record the test run but has not passed record key. This becomes a problem when someone submits a pull request from a forked repository. Example in https://travis-ci.org/bahmutov/cypress-react-unit-test/builds/328887786

$ npm test -- --record
> [email protected] test /home/travis/build/bahmutov/cypress-react-unit-test
> cypress run "--record"
You passed the --record flag but did not provide us your Record Key.
You can pass us your Record Key like this:
  cypress run --record --key <record_key>
You can also set the key as an environment variable with the name CYPRESS_RECORD_KEY.
https://on.cypress.io/how-do-i-record-runs
npm ERR! Test failed.  See above for more details.
The command "npm test -- --record" exited with 1.

This is a very common case for public repos - and every pull request will have tests fail without even running.

Proposal

Maybe a better solution would be for the test runner to

@bahmutov
Copy link
Contributor Author

Examples of pull requests that fail because of --record flag but without record key environment variable

@bahmutov
Copy link
Contributor Author

Ok, version [email protected] should give this information bahmutov/is-fork-pr#4

@bahmutov bahmutov added this to the 2.2.0 milestone Mar 27, 2018
@jennifer-shehane jennifer-shehane modified the milestones: 2.2.0, 3.0.0 Apr 11, 2018
brian-mann pushed a commit that referenced this issue May 16, 2018
* server: do not fail if missing record key from fork PR, close #1193

* remove .only

* fix white space

* add e2e tests around forked PR's warning but running without recording
@jennifer-shehane jennifer-shehane added stage: pending release and removed stage: pending release stage: investigating Someone from Cypress is looking into this stage: needs investigating Someone from Cypress needs to look at this stage: ready for work The issue is reproducible and in scope labels May 24, 2018
@brian-mann
Copy link
Member

Released in 3.0.0.

NicholasBoll added a commit to Workday/canvas-kit that referenced this issue Oct 4, 2019
Forks are failing because they do not have access to secrets. Cypress fixed this if `--key` is not used, but an environment variable is instead used: cypress-io/cypress#1193

Hopefully this fixes the issue. If not, we'll have to either use the Cypress key in plain text or disable recordings altogether.
NicholasBoll added a commit to Workday/canvas-kit that referenced this issue Oct 7, 2019
* ci: Fix fork failures

Forks are failing because they do not have access to secrets. Cypress fixed this if `--key` is not used, but an environment variable is instead used: cypress-io/cypress#1193

Hopefully this fixes the issue. If not, we'll have to either use the Cypress key in plain text or disable recordings altogether.
@NicholasBoll
Copy link
Contributor

@bahmutov

It seems fork detection is on a per-CI-provider-basis. We're using Github Actions which still fails since it is not detected. Should the cypress run --record command not fail if no key is present, but instead warn and run in non-record mode? Or perhaps a config option to fail if no key present (or the opposite)?

If that seems reasonable, I can create a proposal.

@bahmutov
Copy link
Contributor Author

bahmutov commented Oct 8, 2019

@NicholasBoll we kind of have this situation in several places - and we always been able to get around it using shell scripting, something like: if env variable CYPRESS_RECORD_KEY then run cypress run --record, otherwise just run cypress run. I don't think just running tests without recording them on the Dashboard is going to work. For example, you might have Dashboard to GitHub integration enabled - and it would not notify out without recording, thus you might be unaware of the test run results. At least with external scripting it is up to you to configure, while adding another option to the Test Runner itself would just make it more complex I feel.

@bahmutov
Copy link
Contributor Author

bahmutov commented Oct 8, 2019

@NicholasBoll maybe I can add GitHub Actions detection support to bahmutov/is-fork-pr#113 - then it will propagate into the Test Runner and do what you want?

@NicholasBoll
Copy link
Contributor

NicholasBoll commented Oct 8, 2019

For now I'm just pretending to be Travis by setting Travis variables. You're right that the utility of Cypress is decreased without using the Dashboard. CI providers are trying to mature. CircleCI seems to be the most mature in terms of security and features. Cypress uses CircleCI and allows the Cypress key to be used in forks. It is still possible to revoke access to that key unless you use their new security contexts and involve a manual step to trust forks.

Github Actions is not that mature yet. I have an open suggestion: https://github.jparrowsec.cnmunity/t5/GitHub-Actions/Make-secrets-available-to-builds-of-forks/m-p/33876/highlight/true#M1691

For now, I guess we'll just allow the key to be discoverable to those who really want to find it... If it becomes a problem, we'll revoke that key and make a new one. Hopefully soon enough, Github Actions will be mature enough to allow secret sharing with trusted actions.

@bahmutov
Copy link
Contributor Author

bahmutov commented Oct 9, 2019

@NicholasBoll sure, but CircleCI has to explicitly allow environment variables to be passed to forked PRs so it is a security risk. Also, the security contexts are actually a weird thing on CircleCI, see our problem in #5312 - they stop entire job from running; there is no "run the job without context for external users" option.

I like your hack of mimicking Travis CI to detect forked PR. I have opened bahmutov/is-fork-pr#114 to allow you to do this simpler.

Also, on a personal note, I don't find typical CIs to improve much, except for CircleCI Orbs. Only other systems like GitHub Actions, AWS Amplify that build on top of CIs seem to be innovative.

blue-git-pro added a commit to blue-git-pro/canvas-kit that referenced this issue Nov 5, 2021
* ci: Fix fork failures

Forks are failing because they do not have access to secrets. Cypress fixed this if `--key` is not used, but an environment variable is instead used: cypress-io/cypress#1193

Hopefully this fixes the issue. If not, we'll have to either use the Cypress key in plain text or disable recordings altogether.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli type: enhancement Requested enhancement of existing feature type: user experience Improvements needed for UX
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants