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

Add a workflow that builds the docs and deploys them at staged or production #104

Merged
merged 12 commits into from
Dec 7, 2022

Conversation

martin-g
Copy link
Member

@martin-g martin-g commented Dec 5, 2022

Which issue does this PR close?

Closes #39.

Rationale for this change

Publish the documentation for the Datafusion Python bindings.

What changes are included in this PR?

A new GitHub Actions workflow is introduced that builds the docs and deploys them at the appropriate site

Are there any user-facing changes?

No API changes!

@martin-g
Copy link
Member Author

martin-g commented Dec 5, 2022

@andygrove Before merging this PR please create a new branch named asf-staging from asf-site and update its .asf.yaml file contents to be the same as in https://github.com/martin-g/arrow-datafusion-python/blob/asf-staging/.asf.yaml.
It will be used for the staging deployment.

@martin-g
Copy link
Member Author

martin-g commented Dec 5, 2022

@kou Please review!

.github/workflows/docs.yaml Show resolved Hide resolved
.github/workflows/docs.yaml Outdated Show resolved Hide resolved
.github/workflows/docs.yaml Outdated Show resolved Hide resolved
- name: Copy & push the generated HTML
run: |
cd docs-target
find ./ -name "*.html" -type f -print0 | xargs -0 /bin/rm -f
Copy link
Member

Choose a reason for hiding this comment

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

This may not work for other extensions such as *.css. How about using rsync?

rsync \
  -a \
  --delete \
  --exclude '/.git/' \
  ../docs/build/html/ \
  ./

Copy link
Member Author

Choose a reason for hiding this comment

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

Right! There are .css/.js/images/... !
I have to test your approach!
The important thing is to preserve anything that is not autogenerated, especially .asf.yaml

Copy link
Member

Choose a reason for hiding this comment

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

How about copying .asf.yaml from master to asf-site?
apache/arrow-site uses this approach: apache/arrow-site#116

Copy link
Member Author

Choose a reason for hiding this comment

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

This will work for asf-site, but it will break asf-staging, because it uses staging: instead of publish:

Copy link
Member Author

Choose a reason for hiding this comment

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

find ./ -not -name ".asf.yaml" | grep -v ".git" | xargs rm -rf seems to do the trick.

Copy link
Member

Choose a reason for hiding this comment

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

This will work for asf-site, but it will break asf-staging, because it uses staging: instead of publish:

Can we write both staging: and publish: in master's .asf.yaml?

staging:
  whoami: asf-staging
  subdir: datafusion-python
  
publish:
  whoami: asf-site
  subdir: datafusion-python

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this should work!
The only way to find out is to try it! :-)

Copy link
Member

Choose a reason for hiding this comment

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

I think so too. :-)

Copy link
Member Author

@martin-g martin-g Dec 6, 2022

Choose a reason for hiding this comment

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

done with d5e32a2!
depends on #107

.github/workflows/docs.yaml Outdated Show resolved Hide resolved
.github/workflows/docs.yaml Outdated Show resolved Hide resolved
.github/workflows/docs.yaml Outdated Show resolved Hide resolved
@martin-g martin-g force-pushed the docs-workflow branch 2 times, most recently from 52bcc20 to 64dc8d9 Compare December 6, 2022 20:38
Use special email address for the Git user, so that it has an
avatar/icon.

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

This almost looks good to me. I think that we can merge this after we confirm that this works with 0.8.0-rc1 and 0.8.0 tags on your fork. (We need to merge this branch to master on your fork temporary to test these tags.)

.github/workflows/docs.yaml Outdated Show resolved Hide resolved
on:
push:
branches:
- docs-workflow
Copy link
Member

Choose a reason for hiding this comment

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

We need to change this to master when we merge this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right! I used it to be able to test on my fork!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

Comment on lines +29 to +34
- name: Checkout docs target branch
uses: actions/checkout@v3
with:
fetch-depth: 0
ref: ${{ steps.target-branch.outputs.value }}
path: docs-target
Copy link
Member

Choose a reason for hiding this comment

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

It seems that we need to prepare asf-site and asf-staging branches for this: https://github.com/martin-g/arrow-datafusion-python/actions/runs/3633415170/jobs/6130403408

How about using git worktree instead?

diff --git a/.github/workflows/docs.yaml b/.github/workflows/docs.yaml
index 806202a..bfafafa 100644
--- a/.github/workflows/docs.yaml
+++ b/.github/workflows/docs.yaml
@@ -26,12 +26,6 @@ jobs:
           fi
       - name: Checkout docs sources
         uses: actions/checkout@v3
-      - name: Checkout docs target branch
-        uses: actions/checkout@v3
-        with:
-          fetch-depth: 0
-          ref: ${{ steps.target-branch.outputs.value }}
-          path: docs-target
       - name: Setup Python
         uses: actions/setup-python@v4
         with:
@@ -61,7 +55,12 @@ jobs:
       - name: Copy & push the generated HTML
         run: |
           set -x
+          git worktree add docs-target
           cd docs-target
+          if ! git checkout ${{ steps.target-branch.outputs.value }}; then
+            git checkout --orphan ${{ steps.target-branch.outputs.value }}
+            git rm -rf .
+          fi
           # delete anything but: 1) '.'; 2) '..'; 3) .git/
           find ./ | grep -vE "^./$|^../$|^./.git" | xargs rm -rf
           cp ../.asf.yaml .

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO this is more complex/confusing than the current solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

A task before merging this PR is to create asf-staging branch.

Copy link
Member

Choose a reason for hiding this comment

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

.github/workflows/docs.yaml Outdated Show resolved Hide resolved
martin-g and others added 2 commits December 7, 2022 22:55
Co-authored-by: Sutou Kouhei <[email protected]>
Co-authored-by: Sutou Kouhei <[email protected]>
@martin-g
Copy link
Member Author

martin-g commented Dec 7, 2022

This almost looks good to me. I think that we can merge this after we confirm that this works with 0.8.0-rc1 and 0.8.0 tags on your fork. (We need to merge this branch to master on your fork temporary to test these tags.)

Actually I already tested tags - see testN and test-rcN at https://github.com/martin-g/arrow-datafusion-python/actions
Those tags were created from docs-workflow branch, not from master.

@kou
Copy link
Member

kou commented Dec 7, 2022

This almost looks good to me. I think that we can merge this after we confirm that this works with 0.8.0-rc1 and 0.8.0 tags on your fork. (We need to merge this branch to master on your fork temporary to test these tags.)

Actually I already tested tags - see testN and test-rcN at https://github.com/martin-g/arrow-datafusion-python/actions Those tags were created from docs-workflow branch, not from master.

Could you retry testN after you create asf-site branch on your fork?

@martin-g
Copy link
Member Author

martin-g commented Dec 7, 2022

Could you retry testN after you create asf-site branch on your fork?

Done!
There is no Deploy DataFusion Python site for test-rc10 and there is such for test10.

@kou
Copy link
Member

kou commented Dec 7, 2022

Thanks. Committed: martin-g@34ed8b8

I merge this.

@kou kou merged commit a110049 into apache:master Dec 7, 2022
@martin-g martin-g deleted the docs-workflow branch December 8, 2022 09:38
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.

Publish documentation for Python bindings
2 participants