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

Automatically generate previews for docs PRs #275

Closed
wants to merge 1 commit into from

Conversation

mfisher87
Copy link
Collaborator

This is not super polished yet. The config is a bit messy, all steps related to docs-building are repeated (should be extracted to a composite action), and there are too many jobs displayed in the PR "Checks" section.

There are two paths for this workflow:

  1. A PR is opened from a local branch (e.g. by maintainer): "Local" branch documentation change test mfisher87/earthaccess#4
  2. A PR is opened from a fork (by any community member): Fork documentation change test mfisher87/earthaccess#8

For path #1, things are simple; just build the docs and trigger the deploy. The workflow will have access to all the secrets it need.s

For path #2, things are complicated! pull_request events from forks don't have access to repo secrets for security reasons. pull_request_target events don't have access to the changes proposed on the fork for security reasons. One alternative is to trigger the workflow with a comment, and that's the route I took (based on quarto-dev/quarto-web project), and tried to make it convenient.

This is working on my fork because I've populated the Netlify secrets in my fork. This repo (or NSIDC org) needs to follow the Netlify setup procedure (see additions to contributing docs) and populate the secrets. I can set up the Netlify account with my work GitHub account @MattF-NSIDC if that helps!

@github-actions
Copy link

github-actions bot commented Aug 6, 2023

Binder 👈 Launch a binder notebook on this branch for commit 0369b4e

I will automatically update this comment whenever this PR is modified

Copy link
Collaborator

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Nice @mfisher87. It'd be great to have doc previews for PRs here. FWIW readthedocs also has support for building and previewing docs on PRs https://docs.readthedocs.io/en/stable/pull-requests.html. That said, there may be totally valid reasons to go with Netlify -- just thought I'd mention RTD as one possible option.

@MattF-NSIDC
Copy link

MattF-NSIDC commented Aug 7, 2023

Yeah, I really like that feature of RTD! That's what motivated me to build this. I've used RTD with Sphinx before, and it looks like it supports MkDocs, but I haven't looked any closer than that. Will it support everything we're doing, including rendering the notebooks at build-time? 🤔

We could double-publish the docs to GitHub Pages and RTD if that's the case (and get previews for free from RTD)...

I put this together assuming that we wanted to stick with GitHub Pages as our main documentation site, and that's probably an assumption worth challenging. @betolink, what do you think?

EDIT: I apologize for the multiple-account thing. I'm @mfisher87 too. I regret having two accounts.

@jrbourbeau
Copy link
Collaborator

It looks like @betolink set up readthedocs for this repo at some point https://readthedocs.org/projects/earthdata

I should have looked closer earlier, there's already a readthedocs config file in the repo https://github.com/nsidc/earthaccess/blob/main/readthedocs.yml

Maybe flipping the PR preview switch in readthedocs will "just work"?

@MattF-NSIDC
Copy link

I should have looked closer earlier

🤣 Me too! Nice find.

It looks like it may be deployed to RTD under its old name, though: https://earthdata.readthedocs.io/en/latest/

I'm getting a 404 for earthacess.readthedocs.io. I agree, flipping the switch and letting RTD render these previews is miles better than maintaining all this :)

@MattF-NSIDC
Copy link

Got hold of Luis on Slack and he's busy with IceSat2 Hackweek. I'll need him to grant permissions to edit the RTD setup, maybe can do that this evening. If we can just flip that switch in the admin panel, I'll decline this way-overcomplicated PR. We can treat the RTD URL being out-of-date as a separate issue.

@MattF-NSIDC
Copy link

We fixed RTD :)

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.

4 participants