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

Explicitly disable secrets in forked PRs #445

Merged
merged 1 commit into from
Jan 23, 2017

Conversation

jakirkham
Copy link
Member

Seems that CircleCI was using a deploy key with forked PR builds. However, they were not exposing the secret environment variables before. Though now it appears that they are exposing secret environment variables too. This is a bit strange as the CircleCI defaults to having the setting for secret environment variables disabled by default. In other words, it appears this default is not respected by CircleCI currently.

As a workaround, we seem to be able to fix this by explicitly disabling secret environment variables as well. A little bit of testing verifies this works as intended. While it is a workaround, it is a good thing to keep should CircleCI ever change the default value of this setting. Thus we have added this change to the configuration of CircleCI on new feedstocks right after project creation in this commit.

Should note this also uses a non-public portion of the API as does enabling PRs from forks. However, we have gotten assurances from CircleCI before that this is ok to use.

Seems that CircleCI was using a deploy key with forked PR builds.
However, they were not exposing the secret environment variables before.
Though now it appears that they are exposing secret environment
variables too. This is a bit strange as the CircleCI defaults to having
the setting for secret environment variables disabled by default. In
other words, it appears this default is not respected by CircleCI
currently.

As a workaround, we seem to be able to fix this by explicitly disabling
secret environment variables as well. A little bit of testing verifies
this works as intended. While it is a workaround, it is a good thing to
keep should CircleCI ever change the default value of this setting. Thus
we have added this change to the configuration of CircleCI on new
feedstocks right after project creation in this commit.

Should note this also uses a non-public portion of the API as does
enabling PRs from forks. However, we have gotten assurances from
CircleCI before that this is ok to use.
@jakirkham
Copy link
Member Author

cc @isuruf

response = requests.put(url, headers=headers, json={'feature_flags':{'forks-receive-secret-env-vars':False}})
if response.status_code != 200:
response.raise_for_status()
# Enable CircleCI builds on forked PRs.
response = requests.put(url, headers=headers, json={'feature_flags':{'build-fork-prs':True}})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to send both options as one request?

Copy link
Member Author

@jakirkham jakirkham Jan 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly. Also wanted to make sure that they were done in this order just in case.

Edit: Fixed grammar.

@jakirkham
Copy link
Member Author

Thanks @isuruf. I'm going to put this in version 2.0.1 ASAP. Just need to do a brief review of master to see what else will be released with it.

Also have ran this on all existing feedstocks. As a quick final test, PR ( conda-forge/ijroi-feedstock#2 ) demonstrates this is now working correctly. Please let me know if you see anything that isn't.

@jakirkham jakirkham merged commit 850bc4d into conda-forge:master Jan 23, 2017
@jakirkham jakirkham deleted the circleci_disable_secrets branch January 23, 2017 05:32
@jakirkham
Copy link
Member Author

Added a few more PRs of my own to verify this was acting correctly and it seems to be ok.

@jakirkham
Copy link
Member Author

This is being released ATM. Linux releases, which staged-recipes and the re-rendering service rely on, are already released. Also Mac releases are out. Windows releases are queued.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants