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

Only restrict to deploying on pushes to master in the python/peps repo #1996

Merged
merged 1 commit into from
Jan 21, 2022

Conversation

AA-Turner
Copy link
Member

cc: @pablogsal

Relaxes restriction from #1987, builds on PRs from forks.

@AA-Turner AA-Turner requested a review from a team as a code owner June 21, 2021 16:20

jobs:
deploy-to-pages:
runs-on: ubuntu-latest

# If on the python/peps repo, only deploy on pushes to `master`. Otherwise, build
Copy link
Member

Choose a reason for hiding this comment

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

I am not a fan of this, unfortunately. I don't like all these hardcoded values. If users want to deploy from forks they can maybe do it from other branches

Copy link
Member Author

Choose a reason for hiding this comment

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

It is hardcoded, but shouldn't change? (barring branch renames of course)

Can have a think of other ways to do it but this was the cleanest I came up with last night to restrict deploys to master on this repo, whilst being liberal on forks (only had the time to test just now)

Copy link
Member Author

Choose a reason for hiding this comment

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

Also noting the on: target will only deploy on master currently for this repo and forks.

Copy link
Member

@pablogsal pablogsal Jun 21, 2021

Choose a reason for hiding this comment

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

whilst being liberal on forks (only had the time to test just now)

But forks can do whatever they want on their master branch, can't they?

As in, they can make a commit that customizes this file on their fork only

Copy link
Member Author

Choose a reason for hiding this comment

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

True, perhaps I'm overthinking this in trying to make it easy (and people have coped up until now without rendered previews I guess).

This is less important than the theming work in 1933 though anyway, and was a (perhaps poor) attempt at a quality of life improvement :)

Copy link
Member

Choose a reason for hiding this comment

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

In general, I would like my fork to behave as closely as possible to upstream, so I have a better idea of the effect of my changes would have on upstream before opening a PR.

So it would be nice to have fork deploys too, as long as they work out of the box without needing to set any deploy keys, and don't fail my CI.

Some projects use for example secret credentials to deploy to S3, which would fail for forks and would need skipping. Not the case with gh-pages?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the idea here is that it works out of the box with the github integration - no external services / arcanery are neeeded.

E.g. I've been deploying previews all the way through the sphinx integration on https://aa-turner.github.io/peps/ using something similar to what I proposed here (but more hacky).

Three caveats I can think of:

  • (a) I don't know if when GH Pages is enabled on this repo, if it will pull through automatically to forks (otherwise people would need to go to the pages tab of repo settings to enable)
  • (b) Only one PR would preview at any one time, and it would be the latest pushed
  • (c) Unlike e.g. the netifly bot, there is currently no nice automatic comment to point out the preview deploy (I think that this could be done though via the same actions script -- haven't tried)

As I said above, this is a much more trivial píece of work than the final Sphinx PR, and is definitely non-core work in the Sphinx transition process.

A

@AA-Turner AA-Turner mentioned this pull request Jun 29, 2021
@AA-Turner
Copy link
Member Author

Hijacking this PR to test Sphinx on PRs, but not deploy.

@AA-Turner AA-Turner force-pushed the sphinx-deploy-forks branch from 9fbc332 to 849691e Compare January 21, 2022 11:33
@AA-Turner AA-Turner force-pushed the sphinx-deploy-forks branch from 849691e to 7e364de Compare January 21, 2022 11:34
@AA-Turner AA-Turner merged commit fb046e1 into python:main Jan 21, 2022
@AA-Turner AA-Turner deleted the sphinx-deploy-forks branch January 21, 2022 11:37
@AA-Turner AA-Turner restored the sphinx-deploy-forks branch January 21, 2022 11:37
@AA-Turner
Copy link
Member Author

Forgot to update the PR title (not the end of the world but mildly annoying) -- as shown here the Sphinx job now follows the build job more closely and runs on PRs, but skips the deploy step

A

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

Successfully merging this pull request may close these issues.

4 participants