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 deploy forked repositories #313

Merged
merged 2 commits into from
Feb 18, 2020
Merged

Conversation

dhimmel
Copy link
Member

@dhimmel dhimmel commented Feb 18, 2020

It looks like GitHub actions was also running on my fork. Now GitHub Actions deploys even without a manually set SSH Key. Therefore, it tried to deploy on my fork.

@dhimmel
Copy link
Member Author

dhimmel commented Feb 18, 2020

When will users want to deploy a manuscript to GitHub Pages? In general, I think they'll only want to deploy the upstream manuscript in case of forks. Although this may not always be the case. Sometimes a fork may want to deploy as well. Perhaps we could default to only deploying non-fork repos with a way to override and deploy a fork.

@agitter
Copy link
Member

agitter commented Feb 18, 2020

I agree that the default should be that forks do not deploy. Users may want to build the manuscript in a fork to test changes as long as it doesn't deploy.

@dhimmel
Copy link
Member Author

dhimmel commented Feb 18, 2020

Users may want to build the manuscript in a fork to test changes as long as it doesn't deploy.

The problem is with our current configuration, we have:

on:
  push:
    branches:
      - master
  pull_request:
    branches:
      - master

So we're not building branches on forks, which is the main place users would be testing these changes.

I propose merging this PR as a quick fix since it will prevent failing fork master builds. And then thinking more about the ideal configuration.

@dhimmel
Copy link
Member Author

dhimmel commented Feb 18, 2020

For reference, I dumped the github actions context in this workflow run https://github.com/dhimmel/rootstock-actions-deploy/runs/453060191

Copy link
Member

@agitter agitter left a comment

Choose a reason for hiding this comment

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

I propose merging this PR as a quick fix

Works for me. I wasn't sure why both conditional statements are needed, but if it has the intended behavior that's good enough for me.

@dhimmel
Copy link
Member Author

dhimmel commented Feb 18, 2020

Another possibility is that we could use the github.repository.has_pages context variable like:

'!github.repository.fork || github.repository.has_pages

Works for me. I wasn't sure why both conditional statements are needed, but if it has the intended behavior that's good enough for me.

I will remove the first. Was for testing, but didn't actually test anything.

@agitter
Copy link
Member

agitter commented Feb 18, 2020

What's the default state of github.repository.has_pages for a forked repository? Is that false initially?

@dhimmel dhimmel merged commit 9bdf4c1 into manubot:master Feb 18, 2020
@dhimmel dhimmel deleted the skip-fork-actions branch February 18, 2020 16:08
@dhimmel
Copy link
Member Author

dhimmel commented Feb 18, 2020

See https://github.com/dhimmel/manubot-rootstock/actions/runs/41309705 for the fork action on the merged commit.

@agitter
Copy link
Member

agitter commented Feb 18, 2020

It still tried to deploy?

@dhimmel
Copy link
Member Author

dhimmel commented Feb 18, 2020

What's the default state of github.repository.has_pages for a forked repository? Is that false initially?

It seems that forking on github does replicate the gh-pages and output branches. However, the GitHub Pages build does not run the branch until another commit is made such that there is not webpage available. Not sure what has_pages would be but guessing it's true.

Update: actually it's false upon fork. I forked to cognoma/rootstock and if I do https://api.github.com/repos/cognoma/rootstock, I see:

  "has_pages": false,

will delete cognoma/rootstock

It still tried to deploy?

ahh. will debug

@dhimmel
Copy link
Member Author

dhimmel commented Feb 18, 2020

Hopefully fixed in dhimmel@cd8ce12. If the deployment is properly skipped I will push this commit to manubot/rootstock:master.

Skipped in https://github.com/dhimmel/manubot-rootstock/runs/453222057

ploegieku added a commit to ploegieku/2023-functional-homology-paper that referenced this pull request Aug 6, 2024
merges manubot/rootstock#313

Previously deployment was governed by the presence of
MANUBOT_SSH_PRIVATE_KEY, but now that GITHUB_TOKEN
is supported for deployment, forks with master branch commits
attempted to deploy.
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