GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-37860: re-add netlify.toml to set up deploy previews for docs by epicfaace · Pull Request #92852 · python/cpython

Conversation

@epicfaace

@epicfaace epicfaace commented May 16, 2022

Copy link
Copy Markdown
Contributor

Fixes #82041 by setting up Netlify to set up deploy previews for docs.

This PR re-does #15288. Reverts #30272 (where we had originally removed netlify.toml due to build limits). As per #82041 (comment), the build limits are now large enough that they should be fine to use for cpython.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

CAM-Gerlach reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@epicfaace
Revert "bpo-46184: remove netlify.toml (python#30272)"
6a80a21
This reverts commit fbaf2e6.
@bedevere-bot bedevere-bot added the awaiting review label May 16, 2022
@epicfaace

epicfaace commented May 16, 2022

Copy link
Copy Markdown
Contributor Author

Do we need a NEWS label for this? I was thinking we could skip news?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced May 16, 2022
Add deploy preview for docs #82041
Closed
bpo-37860: Enable docs deploy previews through GitHub Actions #19025
Closed
@hugovk hugovk added the skip news label May 16, 2022
@hugovk

hugovk commented May 16, 2022

Copy link
Copy Markdown
Member

Thanks for this!

I added the skip news label.

Please could you do a deploy to a demo site first to check the command still works three years later?

For example, Ubuntu Focal is now the default image, and the default Python is 3.8 (not 2.7) so may be able to skip Doc/runtime.txt altogether.

https://github.com/netlify/build-image/blob/focal/included_software.md

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

epicfaace added 5 commits May 16, 2022 15:25
@epicfaace
Delete runtime.txt
22a55a5
@epicfaace
Create runtime.txt
d2b7cb4
@epicfaace
Delete runtime.txt
b2a5dd2
@epicfaace
Update netlify.toml
882fd4c
@epicfaace
Update netlify.toml
adbd5dd
@epicfaace

epicfaace commented May 16, 2022

Copy link
Copy Markdown
Contributor Author

@hugovk made some small fixes, it now deploys correctly. My branch is deployed at: https://steady-heliotrope-731cac.netlify.app/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hugovk

hugovk commented May 16, 2022

Copy link
Copy Markdown
Member

Good work! Out of interest, how long does a single build/deploy take?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@epicfaace

epicfaace commented May 16, 2022

Copy link
Copy Markdown
Contributor Author

It took 4m16s.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hugovk

hugovk commented May 17, 2022

Copy link
Copy Markdown
Member

Thanks, so with 25,000 minutes per month, 4-5 minutes per build gives us 5,000-6,250 builds per month.

That's significantly better than the ~170/month estimate 2 years ago: #15288 (comment).

I see it's also possible to configure ignored builds, can we only build previews when there are changes in the Doc directory? That would save resources and make PR runs quicker/cleaner too.

https://docs.netlify.com/configure-builds/ignore-builds/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@epicfaace

epicfaace commented May 17, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

I see it's also possible to configure ignored builds, can we only build previews when there are changes in the Doc directory? That would save resources and make PR runs quicker/cleaner too.

https://docs.netlify.com/configure-builds/ignore-builds/

This is already configured by default. The link above says "Depending on your Branches settings, any time there is a change in a linked repository, Netlify tries to determine whether there are changes in the site’s base directory by comparing the last known version of the files in that directory."

The base directory is already set to Doc/.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hugovk

hugovk commented May 17, 2022

Copy link
Copy Markdown
Member

Great, then I think we're all set!


@ewdurbin As a follow on to #15288 (comment) in 2019, we should now have 25k minutes per month (#82041 (comment)), and are ready to try this out again.

Please could you hook up the Netlify account? I would suggest we only run it on main to begin, and review usage after a few weeks/a month, and consider enabling it for 3.11 too (possibly even others).


If we have max 3 team members (#15288 (comment)), @Mariatta, you asked to be added in 2019 (#15288 (comment)), would you still like to?

And to further increase the bus factor, who would like to be the 3rd?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@epicfaace

epicfaace commented May 17, 2022

Copy link
Copy Markdown
Contributor Author

I'm happy to be the third! If it's fine though I'm not a committer

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@epicfaace

epicfaace commented May 17, 2022

Copy link
Copy Markdown
Contributor Author

Also, looks like we're no longer limited to 3 team members max: see https://www.netlify.com/legal/open-source-policy/:

Our Open Source plan has the same features and limits as our Pro plan, with the addition of free unlimited team members.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@epicfaace

epicfaace commented May 17, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Also, one more thing: I think to comply with the requirements mentioned in https://www.netlify.com/legal/open-source-policy/, we should add a Netlify badge / "This site is powered by Netlify" message on all deploy previews.

I don't think we need to / should add it to the main docs, but we can add it on deploy previews only -- for example, we could set an environment variable that then renders the Netlify badge only on deploy previews. I'm happy to implement if we're good with that approach. What do you think @hugovk ?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hugovk

hugovk commented May 18, 2022

Copy link
Copy Markdown
Member

The env var approach sounds good to add it only to deploy previews. Perhaps the text link option, in the footer? And just on the homepage or all pages?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Mariatta

Mariatta commented May 18, 2022

Copy link
Copy Markdown
Member

Thanks for your work @epicface.

I think we need to display more prominently that the docs rendered is a "preview". So where-ever we put the netlify badge, we should add the "preview" info as well. Is that something we can do?

As comparison, on the devguide (built with readthedocs), the preview is displayed at the top of the page:

e.g: https://cpython-devguide--867.org.readthedocs.build/

Screen Shot 2022-05-18 at 10 30 25 AM

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@AA-Turner

AA-Turner commented May 21, 2022

Copy link
Copy Markdown
Member

Adding a warning admonition to the top of the page seems a reasonable idea regardless, although the ephemeral nature of the previews (& non-canonical URL) should help to disambiguate d.p.o from the previews.

@hugovk from the linked Netifly page, it seems only the "main page" is required - so /index.html.

A

hugovk reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hugovk

hugovk commented Jun 18, 2022

Copy link
Copy Markdown
Member

Hi again @epicfaace! Do you think you can make the above changes? Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@arhadthedev

arhadthedev commented Jun 18, 2022

Copy link
Copy Markdown
Member

I think the popup warning that collapses into the bottom will be more noticeable. Here is an example from tc39/ecma262:

image

image

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@epicfaace

epicfaace commented Jun 19, 2022

Copy link
Copy Markdown
Contributor Author

Hi again @epicfaace! Do you think you can make the above changes? Thanks!

Yes, doing so!

@arhadthedev I like that idea. relevant code here: https://github.com/tc39/ecma262/blob/267fdbdc716677bc9934906a498cf9bad4bfb70d/scripts/snapshot_warning.html https://github.com/tc39/ecma262/blob/267fdbdc716677bc9934906a498cf9bad4bfb70d/scripts/insert_snapshot_warning.js and https://github.com/tc39/ecma262/blob/267fdbdc716677bc9934906a498cf9bad4bfb70d/scripts/snapshot_warning.css

arhadthedev reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@epicfaace
Add netlify badge
e3321c6
@epicfaace

epicfaace commented Jun 19, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

All right, this PR should be ready now, @Mariatta and @hugovk . I just added a simple banner for now that displays on top of all pages:

image

Sample deploy preview: https://62af84a185fcf70008d07339--steady-heliotrope-731cac.netlify.app/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hugovk
hugovk reviewed Jun 19, 2022
View reviewed changes

@hugovk hugovk left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks, a couple of comments!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/tools/templates/layout.html Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/tools/templates/layout.html Resolved

Uh oh!

There was an error while loading. Please reload this page.

@epicfaace @hugovk
Update Doc/tools/templates/layout.html
3268970
Co-authored-by: Hugo van Kemenade <[email protected]>
@hugovk

hugovk commented Jun 19, 2022

Copy link
Copy Markdown
Member

Om mobile, the top of the banner is hidden:

image

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@epicfaace
Update layout.html
9a9513c
@epicfaace

epicfaace commented Jun 19, 2022

Copy link
Copy Markdown
Contributor Author

Om mobile, the top of the banner is hidden:

Note that this banner uses the same styling as the "This document is for an old version of Python that is no longer supported" banner. The top of this banner will also be hidden once we start showing this banner on versions of the docs that have a responsive mobile layout. This means that when 3.8 reaches EOL (as 3.8 is the first version of the docs with a responsive mobile layout), we'll have a similar problem in which the top of the banner "This document is for an old version of Python that is no longer supported." will be hidden.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@epicfaace

epicfaace commented Jun 19, 2022

Copy link
Copy Markdown
Contributor Author

to clarify -- 3.6 doesn't have this problem as it's not using a mobile responsive layout:

image

but 3.8 most certainly will, once we add the red banner.

image

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hugovk

hugovk commented Jun 19, 2022

Copy link
Copy Markdown
Member

Ah okay. So for the "no longer supported" it won't happen until 3.8 is EOL on 2024-10-14, but for the previews it will happen right away :)

We could report this to (I guess) https://github.com/python/python-docs-theme and come up with a fix later?

epicfaace reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@epicfaace epicfaace mentioned this pull request Jun 19, 2022
Documentation with responsive theme (3.8+) will hide "not supported" banner behind navbar for mobile layouts python/python-docs-theme#95
Closed
@epicfaace

epicfaace commented Jun 19, 2022

Copy link
Copy Markdown
Contributor Author

@hugovk yes. It seems more involved to fix that issue so I've went ahead and reported it in python/python-docs-theme#95.

hugovk reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hugovk
hugovk approved these changes Jun 20, 2022
View reviewed changes

@hugovk hugovk left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Jun 20, 2022
@hugovk

hugovk commented Jul 27, 2022

Copy link
Copy Markdown
Member

@ewdurbin Hi! Please could you hook up the Netlify account again?

See #92852 (comment)

And please add @Mariatta to the team. Thank you!

CAM-Gerlach reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou

encukou commented Aug 22, 2022

Copy link
Copy Markdown
Member

@ewdurbin Will you be able to do this, or is something blocking you?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@CAM-Gerlach CAM-Gerlach mentioned this pull request Nov 8, 2022
PEP 8104: 2023 Term Steering Council election python/peps#2875
Merged
@ewdurbin
ewdurbin merged commit 85d5a7e into python:main Dec 6, 2022
@bedevere-bot bedevere-bot removed the awaiting core review label Dec 6, 2022
@hugovk

hugovk commented Dec 6, 2022

Copy link
Copy Markdown
Member

Looks like we're all set now!

Here's a recent docs PR build:

  • gh-87452 Making returncode unreliability explicit #92181

And the corresponding preview:

  • And https://deploy-preview-92181--python-cpython-preview.netlify.app/

Thank you Ee for setting up Netlify and Ashwin for this and the earlier PRs! 👏

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou

encukou commented Dec 6, 2022

Copy link
Copy Markdown
Member

The banner is missing, though.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hugovk

hugovk commented Dec 6, 2022

Copy link
Copy Markdown
Member

I guess because that PR doesn't yet have the banner changes from this one.

@encukou Would you like to rebase/merge #100054 from main to test it?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou

encukou commented Dec 6, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

You're right, with an up-to-date branch it works!

hugovk reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Mariatta

Mariatta commented Dec 6, 2022

Copy link
Copy Markdown
Member

This is amazing! Thank you everyone for pushing through and thanks @epicfaace for this PR. This is a great help to all contributors here.

hugovk reacted with rocket emoji
All reactions
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@epicfaace

epicfaace commented Dec 6, 2022

Copy link
Copy Markdown
Contributor Author

Amazing, thank you all for reviewing and getting this in!

hugovk reacted with rocket emoji
All reactions
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn sobolevn mentioned this pull request Dec 7, 2022
Netlify builds happen for PRs that do not change docs #100072
Closed
@CAM-Gerlach CAM-Gerlach mentioned this pull request Dec 16, 2022
Previews in pull requests python/python-docs-theme#70
Closed
@hugovk hugovk mentioned this pull request Dec 18, 2022
Docs: Don't upload CI artifacts #100330
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hugovk hugovk hugovk approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add deploy preview for docs

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@epicfaace @hugovk @Mariatta @AA-Turner @arhadthedev @encukou @ewdurbin @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.