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

ci: deploy branch preview docs only on main repo #2871

Closed
wants to merge 1 commit into from

Conversation

zonca
Copy link
Collaborator

@zonca zonca commented Dec 6, 2023

Forked repositories normally do not have secrets correctly configured,
so the deploy preview of the docs always fail, making PRs on forks always failing.
Of course this won't work if you have cases where you have forked repos with secrets.

See
https://github.com/orgs/community/discussions/26409#discussioncomment-3251822

Copy link

codecov bot commented Dec 6, 2023

Codecov Report

Merging #2871 (11cdb13) into main (123fa09) will increase coverage by 0.05%.
Report is 1 commits behind head on main.
The diff coverage is 77.88%.

Additional details and impacted files
Files Coverage Δ
src/awkward/contents/bitmaskedarray.py 70.81% <100.00%> (+1.53%) ⬆️
src/awkward/contents/bytemaskedarray.py 89.75% <100.00%> (+0.73%) ⬆️
src/awkward/contents/content.py 74.81% <100.00%> (-0.28%) ⬇️
src/awkward/contents/listoffsetarray.py 82.86% <100.00%> (+0.13%) ⬆️
src/awkward/contents/numpyarray.py 91.18% <100.00%> (ø)
src/awkward/contents/regulararray.py 87.43% <100.00%> (+0.18%) ⬆️
src/awkward/contents/unmaskedarray.py 74.60% <100.00%> (+0.39%) ⬆️
src/awkward/contents/listarray.py 91.78% <83.33%> (+1.06%) ⬆️
src/awkward/contents/unionarray.py 85.22% <75.00%> (-0.25%) ⬇️
src/awkward/contents/indexedarray.py 80.95% <82.35%> (+2.05%) ⬆️
... and 2 more

... and 1 file with indirect coverage changes

@zonca
Copy link
Collaborator Author

zonca commented Dec 6, 2023

apologies just noticed there was already a if statement on that action,
however it does not seem to work:

zonca#6

@zonca
Copy link
Collaborator Author

zonca commented Dec 6, 2023

hopefully this one instead is working:

zonca#7

@agoose77
Copy link
Collaborator

agoose77 commented Dec 6, 2023

We need to think about this - we don't want to leak any secrets to forked PRs. When I looked last, I felt we couldn't use any of the mechanisms for opening permissions without compromising access keys. It might be somewhat viable, I'll take a look this week!

@agoose77
Copy link
Collaborator

Thanks for this PR! I'm going to close it, because there are security concerns with deploying docs from third-party repositories; we don't have any way of ensuring that executable code run during the docs is well-behaved, or that the built assets that are deployed aren't e.g. embedding a cryptominer.

The only way I think we can implement this securely (without requiring people to constantly be vigilant) is to implement a one-off execution for a fork. I don't think there's a convenient way to do that, unfortunately.

@agoose77 agoose77 closed this Feb 13, 2024
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