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

Use Vercel action for continuous documentation #964

Merged
merged 6 commits into from
Feb 26, 2021
Merged

Conversation

seisman
Copy link
Member

@seisman seisman commented Feb 24, 2021

Description of proposed changes

For Continous Documentation (i.e., previewing documentation in PRs), we're currently using the Vercel App. To make the app work, we need two configuration files in the repository root directory, package.json and vercel.json. The Vercel App works well for PyGMT, but is more challenging for GMT (GenericMappingTools/gmt#4848 (comment)).

This PR adds a new workflow using the vercel-action action.

The new Vercel action has the following pros:

  1. package.json and vercel.json are no longer needed. Two fewer files in the root directory
  2. The workflow is easier to understand and maintain. We just need to build the documentation as usual and run the vercel action to deploy it to vercel.
  3. The old vercel app shows "View deployment" button for each build. It adds too much vertical spaces, making a PR page much longer. The new vercel action doesn't have such a problem.
    image

The new Vercel action also has some cons:

  1. After the first build finishes, the action creates a comment, showing the preview URL. For the following builds, the action updates the preview URL in the comment. So PR authors and maintainers will be notified once by the action. It's noisier than the old Vercel App.
    image

  2. The comment only shows the preview URL of the latest build, so it's not easy to compare the documentation of two builds. It's still possible to get old preview URLs by checking the workflow output.

  3. The workflow takes ~7 minutes, slightly slower than the old Vercel App (~6 minutes).

Notes:

You may notice that the Vercel App also creates a comment and still show the "View deployment" buttons, even though we already remove the package.json and vercel.json files. That's simply because we still have the Vercel App installed and enabled for this repository. We should diable the Vercel App if we decide to use the new workflow.

image
image

TODO

  • Update CONTRIBUTING.md and MAINTENANCE.md

Fixes #

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If adding new functionality, add an example to docstrings or tutorials.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

  • /format: automatically format and lint the code
  • /test-gmt-dev: run full tests on the latest GMT development version

@vercel
Copy link

vercel bot commented Feb 24, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/gmt/pygmt/F8Yc5DfYt23Aep1zxaQ3zYViuhKM
✅ Preview: https://pygmt-git-vercel-action-gmt.vercel.app

@seisman seisman changed the title Switch to vercel action Use Vercel action for continuous documentation Feb 25, 2021
@GenericMappingTools GenericMappingTools deleted a comment from github-actions bot Feb 25, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Feb 25, 2021

Deploy preview for pygmt ready!

✅ Preview
https://pygmt-65j2gzl59-gmt.vercel.app

Built with commit 9194155.
This pull request is being automatically deployed with vercel-action

@seisman seisman requested a review from a team February 25, 2021 06:13
@seisman
Copy link
Member Author

seisman commented Feb 25, 2021

Ping @GenericMappingTools/python and @GenericMappingTools/python-maintainers for comments about the new workflow, especially that if you feel that the comment below is annoying.

image

Ping @weiji14 who implemented the old Vercel App, and wrote the two configurations.

Ping @meghanrjones as it's likely that we will use the same action in GMT as mentioned in GenericMappingTools/gmt#4848 (comment).

@seisman seisman added the maintenance Boring but important stuff for the core devs label Feb 25, 2021
@maxrjones
Copy link
Member

I think the new comment structure is a worthwhile trade off for fewer files and easier maintenance. It will be nice to have consistency with continuous documentation between PyGMT and GMT.

Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

A few things to address documentation wise on the "Continuous Documentation" section in MAINTENANCE.md:

  1. Remove references to package.json and vercel.json.
  2. Mention how the secrets VERCEL_ORG_ID, VERCEL_PROJECT_ID and VERCEL_TOKEN are created, and how they can be updated (i.e. link to the Vercel page).

Optional points:

  • Can we set up a matrix build on all 3 OSes in this action? I.e. remove the "Build the documentation" section in ci_tests.yaml and put it here instead.

Rationale: Saves ~4min of CI time. I wanted to separate the tests from the documentation builds in #830, but maybe we can do it in this "Continuous Documentation" workflow instead. I know Leo mentioned using the documentation build as a 'test' before (lazy to dig up that comment), but our >20 min of CI tests is getting wayy too long.

.github/workflows/continuous-documentation.yml Outdated Show resolved Hide resolved
.github/workflows/continuous-documentation.yml Outdated Show resolved Hide resolved
.github/workflows/continuous-documentation.yml Outdated Show resolved Hide resolved
.github/workflows/continuous-documentation.yml Outdated Show resolved Hide resolved
.github/workflows/continuous-documentation.yml Outdated Show resolved Hide resolved
@seisman
Copy link
Member Author

seisman commented Feb 25, 2021

A few things to address documentation wise on the "Continuous Documentation" section in MAINTENANCE.md:

  1. Remove references to package.json and vercel.json.

Will do.

  1. Mention how the secrets VERCEL_ORG_ID, VERCEL_PROJECT_ID and VERCEL_TOKEN are created, and how they can be updated (i.e. link to the Vercel page).

What about writing the instructions in the workflow file, so the workflow is self-contained and the maintenance guide is short.

@weiji14
Copy link
Member

weiji14 commented Feb 25, 2021

  1. Mention how the secrets VERCEL_ORG_ID, VERCEL_PROJECT_ID and VERCEL_TOKEN are created, and how they can be updated (i.e. link to the Vercel page).

What about writing the instructions in the workflow file, so the workflow is self-contained and the maintenance guide is short.

Yep, an inline comment with the Vercel documentation link works too!

Comment on lines +52 to +54
conda install gmt=6.1.1 numpy pandas xarray netcdf4 packaging \
ipython myst-parser sphinx sphinx-copybutton \
sphinx-gallery sphinx_rtd_theme=0.4.3
Copy link
Member Author

Choose a reason for hiding this comment

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

Here I only install the packages required for documentation, to save the building time.

@seisman
Copy link
Member Author

seisman commented Feb 26, 2021

Optional points:

  • Can we set up a matrix build on all 3 OSes in this action? I.e. remove the "Build the documentation" section in ci_tests.yaml and put it here instead.

Rationale: Saves ~4min of CI time. I wanted to separate the tests from the documentation builds in #830, but maybe we can do it in this "Continuous Documentation" workflow instead. I know Leo mentioned using the documentation build as a 'test' before (lazy to dig up that comment), but our >20 min of CI tests is getting wayy too long.

I'm OK with separate workflows for docs and tests. We can try it in a separate PR.

@seisman seisman requested a review from weiji14 February 26, 2021 01:45
@seisman seisman added this to the 0.3.1 milestone Feb 26, 2021
MAINTENANCE.md Outdated Show resolved Hide resolved
MAINTENANCE.md Outdated Show resolved Hide resolved
@seisman seisman marked this pull request as ready for review February 26, 2021 02:26
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

14 lines to 99 lines of code to get rid of 2 files. Hope this is worth it 😆

P.S. Remember to uninstall the Vercel App later @seisman.

@seisman seisman merged commit 17133d1 into master Feb 26, 2021
@seisman seisman deleted the vercel-action branch February 26, 2021 02:49
@seisman
Copy link
Member Author

seisman commented Feb 26, 2021

P.S. Remember to uninstall the Vercel App later.

Just disabled the App in this repository, because it's still used by https://github.com/GenericMappingTools/website.

seisman added a commit that referenced this pull request Feb 26, 2021
@weiji14 weiji14 added the skip-changelog Skip adding Pull Request to changelog label Feb 26, 2021
@weiji14 weiji14 removed this from the 0.3.1 milestone Feb 26, 2021
seisman added a commit that referenced this pull request Feb 26, 2021
* Revert "Disable pull_request_target for the vercel action (#969)"

This reverts commit f455c31.

* Revert "Use Vercel action for continuous documentation (#964)"

This reverts commit 17133d1.
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
- Switch from the Vercel App to the Vercel action
- Remove the old configuration files `package.json` and `vercel.json`
- Update the maintenance guide

Co-authored-by: Wei Ji <[email protected]>
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
…Tools#973)

* Revert "Disable pull_request_target for the vercel action (GenericMappingTools#969)"

This reverts commit f455c31.

* Revert "Use Vercel action for continuous documentation (GenericMappingTools#964)"

This reverts commit 17133d1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs skip-changelog Skip adding Pull Request to changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants