Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Enable fork PR builds. #27

Closed
ryansanford opened this issue Jan 25, 2017 · 2 comments · Fixed by #30
Closed

Enable fork PR builds. #27

ryansanford opened this issue Jan 25, 2017 · 2 comments · Fixed by #30
Assignees

Comments

@ryansanford
Copy link
Member

CircleCI ref : https://circleci.com/docs/fork-pr-builds/

The sensitive variables are only required for the actual gear build post-merge, so we may be able to do this without risk after some tweaks. CircleCI allows enabling building of forked PRs, and not exposing secrets configured for the project within CircleCI.

  1. Don't fail the script when CI vars are not defined or empty, if we're just validating on a PR -- https://github.com/flywheel-io/exchange/blob/master/bin/process-manifests.sh#L34-L37
  2. Don't rely solely on the branch name to process vs. validate, as the source of the fork may come from the master branch. -- https://github.com/flywheel-io/exchange/blob/master/bin/process-manifests.sh#L224
  3. Need to confirm if github deploy key is exposed. CircleCI docs are vague on exposure there when a deploy key has been provided.

This ticket was spurred by a PR from a fork, which I believe, is a workflow we want to encourage. #26

@ryansanford
Copy link
Member Author

@gsfr I welcome your input on feasibility and prioritization with this. Lacking this issue being resolved, we will need to manually validate the PR from carlos.

gsfr pushed a commit that referenced this issue Jan 26, 2017
@gsfr
Copy link
Contributor

gsfr commented Jan 26, 2017

1 is done.
2 is moot, since fork PRs automatically get branch names, such as pull/28.
3 is not a concern because CircleCI's automatically-generated deploy key is read-only and only used for checkouts; we use a machine user, stored in the GIT_REMOTE environment variable, for pushing.

I've turned on fork builds and tested with my own fork. Seems to work and CircleCI prints a reassuring message regarding suppression of environment variables.

#30 should be ready to go.

@gsfr gsfr closed this as completed in #30 Jan 26, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants