Fix deployment of docs to netlify #34984

tobiasdiez · 2023-02-06T15:14:35Z

Normally, on github, netlify takes care of the build + deploy fully automatically. However, our build takes to long so that we would hit the free build mins relatively quickly. So we continue deploying the docs via github actions. But PR workflows don't have access to secrets, so we need to split the upload of the docs in a new workflow, see https://securitylab.github.com/research/github-actions-preventing-pwn-requests/.

Moreover, the netlify deploy cli doesn't take care of adding a PR comment and status check linking to the deployed website (netlify does this automatically if it would build + deploy the page). So we add this functionality manually.

codecov-commenter · 2023-02-06T18:05:02Z

Codecov Report

Base: 88.60% // Head: 88.58% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (0a676e1) compared to base (c000c95).
Patch has no changes to coverable lines.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #34984      +/-   ##
===========================================
- Coverage    88.60%   88.58%   -0.02%     
===========================================
  Files         2136     2136              
  Lines       396142   396142              
===========================================
- Hits        350990   350923      -67     
- Misses       45152    45219      +67

Impacted Files	Coverage Δ
src/sage/categories/super_modules_with_basis.py	`96.29% <0.00%> (-3.71%)`	⬇️
src/sage/homology/matrix_utils.py	`84.40% <0.00%> (-3.67%)`	⬇️
...e/combinat/cluster_algebra_quiver/mutation_type.py	`50.11% <0.00%> (-2.62%)`	⬇️
src/sage/interfaces/qsieve.py	`71.30% <0.00%> (-2.61%)`	⬇️
src/sage/modular/local_comp/liftings.py	`98.33% <0.00%> (-1.67%)`	⬇️
...rc/sage/categories/super_lie_conformal_algebras.py	`95.00% <0.00%> (-1.67%)`	⬇️
...onformal_algebras/lie_conformal_algebra_element.py	`95.52% <0.00%> (-1.50%)`	⬇️
src/sage/schemes/elliptic_curves/hom_velusqrt.py	`94.48% <0.00%> (-1.19%)`	⬇️
src/sage/coding/channel.py	`97.10% <0.00%> (-0.73%)`	⬇️
src/sage/groups/generic.py	`88.34% <0.00%> (-0.68%)`	⬇️
... and 20 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

kwankyu · 2023-02-06T19:08:33Z

Did you do something to automatically add the "Codecov Report" for every PR?

I hate it. It looks like a spam. It is distracting than helpful. I looked at it for some time, but I still don't know what it's doing.

Please turn it off. Or make it one of the checks.

tobiasdiez · 2023-02-07T08:04:18Z

This should do it. However, the "publish docs" workflow need to be included in the default branch to run, so I'm not sure how to test it (maybe in my fork once #34983 is reviewed).

Did you do something to automatically add the "Codecov Report" for every PR?

Yes, this is part of #33355. I agree that the comments are not very helpful and reliable at the moment. But this is due to the rename of the sage repo, which confused codecov. Should be fixed with the next beta release. If you then still have issues, please open a new issue.

mkoeppe

LGTM, nice solution to split it up.
I am assuming that you have tested the deployment workflow.

tobiasdiez · 2023-02-10T13:40:55Z

Sorry, I was a bit too quick with adding the "needs review" label. But now everything should work. A preview can be found at tobiasdiez#2 (with the added comment + status check).

mkoeppe · 2023-02-10T18:24:53Z

.github/workflows/doc-build.yml

@@ -15,14 +15,13 @@ jobs:
 build-docs:
 runs-on: ubuntu-latest
 container: ghcr.io/sagemath/sage/sage-docker-ubuntu-focal-standard-with-targets:dev
- env:
- CAN_DEPLOY: ${{ secrets.NETLIFY_AUTH_TOKEN != '' }}


I think this conditionalization should not be dropped but moved into the new doc-publish workflow.
We don't want the doc-publish workflow to fail on users' repos that have GH Actions enabled.

We don't want the doc-publish workflow to fail on users' repos that have GH Actions enabled.

Why not? It's not doing its intended job and a red workflow doesn't hurt anyone. Don't you think its better to be notified by the failing workflow in case the auth token is not config correctly in the main repo?

If you want to have this kind of monitoring in our main repo, restrict the error to that repo only.

Thanks for your suggestion, but I don't plan to invest more time into what feels like an edge case. If you (or someone else) gets annoyed by the red workflow and you don't want to setup your own netlify deploy, you can simply disable this workflow in the github ui (or change the config in your default branch).

It was already implemented.
Just don't cut it out.

mkoeppe · 2023-02-11T07:13:01Z

Reviewer's patch:

commit 2a17508774e83fd273bc5b76338f4003894f3061 (HEAD -> fix-netlify)
Author: Matthias Koeppe <mkoeppe@math.ucdavis.edu>
Date:   Fri Feb 10 23:08:59 2023 -0800

    .github/workflows/doc-publish.yml: Restore CAN_DEPLOY

diff --git a/.github/workflows/doc-publish.yml b/.github/workflows/doc-publish.yml
index c7be4a46d3..b743ea722e 100644
--- a/.github/workflows/doc-publish.yml
+++ b/.github/workflows/doc-publish.yml
@@ -13,10 +13,13 @@ permissions:
   checks: write
   pull-requests: write
 
+env:
+  CAN_DEPLOY: ${{ secrets.NETLIFY_AUTH_TOKEN != '' }}
+
 jobs:
   upload-docs:
     runs-on: ubuntu-latest
-    if: github.event.workflow_run.conclusion == 'success'
+    if: github.event.workflow_run.conclusion == 'success' && (env.CAN_DEPLOY == 'true' || github.repository == 'sagemath/sage')
     steps:
       - name: Get information about workflow origin
         uses: potiuk/get-workflow-origin@v1_5

tobiasdiez · 2023-02-11T08:05:17Z

Did you tried that? I had something similar: 1c8c63f and ad38d45

Also, this disables the check in other repos that want to enable netlify deploy and be notified about errors...

mkoeppe · 2023-02-11T08:09:56Z

I had something similar: 1c8c63f and ad38d45

Yes, these two cannot work.

mkoeppe · 2023-02-11T08:11:04Z

enable netlify deploy and be notified about errors...

Protecting against the failure mode "someone removed the secret from the repository" is not believable.

tobiasdiez · 2023-02-11T08:24:56Z

enable netlify deploy and be notified about errors...

Protecting against the failure mode "someone removed the secret from the repository" is not believable.

Yes its not very likely. But if the cost is "click two buttons if you are annoyed by the red workflow in a personal fork that you choose to enable the actions in and which has absolutely no negative consequences", then I strongly prefer that. I would appreciate if you could accept this wish to easily test the netlify deployment from the main repo and my fork, or alternatively take over the hosting on netlify yourself.

dimpase · 2023-02-11T16:53:27Z

where are we here now? I'd like build-doc runs like https://github.com/sagemath/sage/actions/runs/4151818607/jobs/7182360271
to really deploy built docs somewhere...

mkoeppe · 2023-02-11T17:51:12Z

I would appreciate if you could accept this wish to easily test the netlify deployment from the main repo and my fork, or alternatively take over the hosting on netlify yourself.

That's a rather inappropriate negotiating tactic.

mkoeppe · 2023-02-11T17:51:37Z

In any case, we need to merge this ASAP.

tobiasdiez · 2023-02-12T04:21:15Z

In any case, we need to merge this ASAP.

Thanks for the review!

Did Checks for this PR uploaded docs somewhere ready for inspection? It's not clear to me. I see from the build-docs log that it did create a docker container with them, but then?

The build-docs uploads the docs as a github artifact (a simple zip file, not a docker container) and then the new publish-docs workflow is trigger. This one then publishes everything to netlify and creates a comment in the PR pointing to the published docs. This can be seen at tobiasdiez#2 with docs at https://deploy-preview-2--sagemath-tobias.netlify.app/ (the url scheme will be the same, with 2 replaced by the PR number).

Please squash this PR instead of merging it, since there are quite a few intermediate testing commits that don't need to be preserved.

dimpase · 2023-02-12T13:30:01Z

Have we converged here? There is still "Changes requested" open.

dimpase · 2023-02-12T13:32:05Z

let's put it in develop, once we agree on what's to be done, still.

mkoeppe · 2023-02-12T14:39:17Z

Have we converged here? There is still "Changes requested" open.

I had already set it to positive review. I've removed the "changes requested" to remove any ambiguity.

tobiasdiez · 2023-02-13T04:40:05Z

let's put it in develop, once we agree on what's to be done, still.

I've merged this now, as this is a purely ci-related PR and we got the thumbs up from @vbraun to merge such PRs.

kwankyu · 2023-02-15T03:01:48Z

In my fork of sagemath/sage, for a PR in the repo, I received this comment

the link in which doesn't work, of course.

In my opinion, this comment should not be sent in the first place.

tobiasdiez · 2023-02-15T04:05:40Z

Seems to be a bug in the netlify action: netlify/actions#67. For now you can disable the workflow in your fork (https://github.com/kwankyu/sage/actions/workflows/doc-publish.yml in the dropdown menu of "..." in the right upper corner)

kwankyu · 2023-02-15T04:12:41Z

Thanks for filing the bug.

tobiasdiez added c: documentation p: blocker / 1 labels Feb 6, 2023

tobiasdiez mentioned this pull request Feb 6, 2023

Fix deployment of docs to netlify #34975

Closed

tobiasdiez added the s: needs review label Feb 7, 2023

tobiasdiez requested a review from mkoeppe February 7, 2023 08:03

tobiasdiez marked this pull request as ready for review February 7, 2023 08:03

mkoeppe approved these changes Feb 7, 2023

View reviewed changes

mkoeppe added s: positive review and removed s: needs review labels Feb 7, 2023

tobiasdiez removed the s: positive review label Feb 8, 2023

tobiasdiez added the s: needs review label Feb 10, 2023

mkoeppe suggested changes Feb 10, 2023

View reviewed changes

mkoeppe added s: needs work and removed s: needs review labels Feb 11, 2023

tobiasdiez added s: needs review and removed s: needs work labels Feb 11, 2023

mkoeppe added s: positive review and removed s: needs review labels Feb 11, 2023

tobiasdiez added 14 commits February 12, 2023 13:30

Fix deployment of docs to netlify

6ac4d2e

fix command

9c21a23

Add separate workflow with privileges

676362e

fix doc publish workflow

751fb79

Increase performance by uploading only one single zip file

3bf6ed8

install zip

5e54493

use env variable in if instead of secret

5ef9372

okay, simply remove the check then

1a416e3

always run publish job

c456d6a

specify output dir for unzip

f936e0b

extend deploy workflow

ac16cd7

fix output reference

56ee2ac

add required permissions

27b1ada

Fix commit sha

0a676e1

dimpase force-pushed the fix-netlify branch from 6c998f7 to 0a676e1 Compare February 12, 2023 13:30

mkoeppe self-requested a review February 12, 2023 14:38

mkoeppe approved these changes Feb 12, 2023

View reviewed changes

This was referenced Feb 12, 2023

Remove 'docker' from the names of the Docker images published on ghcr.io #35079

Merged

Add Hadamard matrices up to order 1000 #35059

Merged

tobiasdiez merged commit 49a0aed into sagemath:develop Feb 13, 2023

mkoeppe added this to the sage-10.0 milestone Feb 13, 2023

kwankyu removed the s: positive review label Feb 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix deployment of docs to netlify #34984

Fix deployment of docs to netlify #34984

tobiasdiez commented Feb 6, 2023 •

edited

Loading

codecov-commenter commented Feb 6, 2023 •

edited

Loading

kwankyu commented Feb 6, 2023 •

edited

Loading

tobiasdiez commented Feb 7, 2023 •

edited

Loading

mkoeppe left a comment

tobiasdiez commented Feb 10, 2023

mkoeppe Feb 10, 2023

tobiasdiez Feb 10, 2023

mkoeppe Feb 10, 2023 •

edited

Loading

tobiasdiez Feb 11, 2023

mkoeppe Feb 11, 2023

mkoeppe commented Feb 11, 2023

tobiasdiez commented Feb 11, 2023

mkoeppe commented Feb 11, 2023

mkoeppe commented Feb 11, 2023

tobiasdiez commented Feb 11, 2023

dimpase commented Feb 11, 2023

mkoeppe commented Feb 11, 2023

mkoeppe commented Feb 11, 2023

tobiasdiez commented Feb 12, 2023 •

edited

Loading

dimpase commented Feb 12, 2023

dimpase commented Feb 12, 2023

mkoeppe commented Feb 12, 2023

tobiasdiez commented Feb 13, 2023

kwankyu commented Feb 15, 2023

tobiasdiez commented Feb 15, 2023

kwankyu commented Feb 15, 2023

Fix deployment of docs to netlify #34984

Fix deployment of docs to netlify #34984

Conversation

tobiasdiez commented Feb 6, 2023 • edited Loading

codecov-commenter commented Feb 6, 2023 • edited Loading

Codecov Report

kwankyu commented Feb 6, 2023 • edited Loading

tobiasdiez commented Feb 7, 2023 • edited Loading

mkoeppe left a comment

Choose a reason for hiding this comment

tobiasdiez commented Feb 10, 2023

mkoeppe Feb 10, 2023

Choose a reason for hiding this comment

tobiasdiez Feb 10, 2023

Choose a reason for hiding this comment

mkoeppe Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

tobiasdiez Feb 11, 2023

Choose a reason for hiding this comment

mkoeppe Feb 11, 2023

Choose a reason for hiding this comment

mkoeppe commented Feb 11, 2023

tobiasdiez commented Feb 11, 2023

mkoeppe commented Feb 11, 2023

mkoeppe commented Feb 11, 2023

tobiasdiez commented Feb 11, 2023

dimpase commented Feb 11, 2023

mkoeppe commented Feb 11, 2023

mkoeppe commented Feb 11, 2023

tobiasdiez commented Feb 12, 2023 • edited Loading

dimpase commented Feb 12, 2023

dimpase commented Feb 12, 2023

mkoeppe commented Feb 12, 2023

tobiasdiez commented Feb 13, 2023

kwankyu commented Feb 15, 2023

tobiasdiez commented Feb 15, 2023

kwankyu commented Feb 15, 2023

tobiasdiez commented Feb 6, 2023 •

edited

Loading

codecov-commenter commented Feb 6, 2023 •

edited

Loading

kwankyu commented Feb 6, 2023 •

edited

Loading

tobiasdiez commented Feb 7, 2023 •

edited

Loading

mkoeppe Feb 10, 2023 •

edited

Loading

tobiasdiez commented Feb 12, 2023 •

edited

Loading