Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

Create a CI setup that allows external Pull Request to build securely #342

Closed
jperata opened this issue Aug 7, 2017 · 6 comments
Closed
Assignees

Comments

@jperata
Copy link
Contributor

jperata commented Aug 7, 2017

Current setup risks credentials setup in Circle CI if we allow builds on external PR's, we should consider an option that merge and setup the tests in an environment were the output is not able to be outputted.

@jperata jperata self-assigned this Aug 16, 2017
@jperata jperata modified the milestone: Production Spokes Aug 16, 2017
@jperata
Copy link
Contributor Author

jperata commented Aug 16, 2017

From circle CI:
https://circleci.com/docs/1.0/fork-pr-builds/
There are two set of permissions, first one is allow fork PR builds or not, second one is allowing the passing of environment variables to that environment.
One option just from that it is to evaluate what tests currently can only run with environment variables on (we already have some tests that are skipped if credentials are not present) , and check how many coverage we have (and increase it as much as possible).
Master will still run with all credential, but that means PR was already reviewed, and it's not going to be released unless master tests pass too.
A different possibility will be too merge external PR's to another branch, have circle test that one too, and then merge to master.
This is still on investigation so this issue is to be expanded with a couple more comments before we start working on it.

@jkelvie
Copy link
Member

jkelvie commented Aug 16, 2017

With the strategy of creating a branch, we then run into the issues with crediting the contributor though, right?

@jperata
Copy link
Contributor Author

jperata commented Aug 16, 2017

not exactly, if we do rebase merging, we end just adding the commits inside in the PR, not one big commit or merge from branch. We will end with a lot of commits, but we can have a policy that commits should be squashed by the user before we been able to merge them.

@jperata
Copy link
Contributor Author

jperata commented Aug 16, 2017

just to clarify, it's not about creating a branch each time a PR comes, instead it's having a parallel branch to master, were we merge external PR's and enable circle ci builds. Then we can generate the new PR more as follow up procedure to merge into master

@jperata
Copy link
Contributor Author

jperata commented Aug 21, 2017

Approach discussed was to have two different CI tools
Travis for building PR's

  • Parallel support to build in all versions of node.
  • No environment variables setup (aws, docker, etc).
  • Coverage will reflect only the tests that can run without vars so it shouldn't have coverage or it should have another coverage tool (coveralls for example).

Circle CI for main coverage and deployment

  • Includes Environment variables
  • Only run in latest node
  • Exclude PR's so that it only runs master, generate coverage for the full test setup.
  • Manages deploys

@jperata
Copy link
Contributor Author

jperata commented Aug 22, 2017

Tested with external PR and works as expected.
Things to check on other issues:

  • Increase coverage without env.
  • gulp docs externally generate docs with user repo instead of bespoken.

@jperata jperata closed this as completed Aug 22, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants