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

Tools: Clean up documentation previews #130

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

codeallthethingz
Copy link
Contributor

@codeallthethingz codeallthethingz commented Jan 11, 2025

This PR creates a new action to delete documentation previews from Readme.com.

This PR also changes the naming structure of the preview URLs to namespace with a username prefix.

This branch is called main2 as I was using that as the main branch in my fork to test the PR closing trigger.

Copy link

github-actions bot commented Jan 11, 2025

📚 Documentation Preview

✅ A preview of the documentation changes in this PR is available for maintainers at:
https://thousandbrainsproject.readme.io/v0.0-main2

Last updated: 2025-01-11 00:42 UTC

@codeallthethingz codeallthethingz added infrastructure Changes to infrastructure triaged This issue or pull request was triaged labels Jan 11, 2025
Copy link
Contributor

@tristanls tristanls left a comment

Choose a reason for hiding this comment

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

Thank you for adding the cleanup 🙌 .

I requested one style change to match the existing arbitrary convention. In addition, I added some comments to consider, but most of them are non-blocking.

runs:
using: 'composite'
steps:
- id: set-vars
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Please change to set_vars to match the existing arbitrary convention.

export PATH="$HOME/miniconda/bin:$PATH"
source activate tbp.monty
echo "monty_version=$(python -m tools.print_version.cli minor)" >> $GITHUB_OUTPUT
echo "BRANCH_NAME=${{ github.event.pull_request.user.login }}-$(echo ${GITHUB_HEAD_REF} | tr -c '[:alnum:]' '-' | sed 's/-*$//')" >> $GITHUB_OUTPUT
Copy link
Contributor

Choose a reason for hiding this comment

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

note (non-blocking): I wonder if passing in the username as explicit input might be more robust than assuming the github.event.pull_request.user.login will always resolve to something where this action is used.

cleanup_docs_preview:
name: cleanup-docs-preview
runs-on: ubuntu-latest
if: github.event.pull_request.merged == true
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (non-blocking): It seems to me that even if the pull request was not merged, the cleanup should occur.

export PATH="$HOME/miniconda/bin:$PATH"
conda --version

- name: Create conda environment
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (non-blocking): Is there a way for you to use a cache here? For example:

- name: Restore cache

- name: Deploy docs
working-directory: tbp.monty
run: |
export PATH="$HOME/miniconda/bin:$PATH"
source activate tbp.monty
export README_API_KEY=${{ secrets.README_API_KEY }}
export IMAGE_PATH=${{ vars.IMAGE_PATH }}
python -m tools.github_readme_sync.cli upload docs "${MONTY_VERSION}-${BRANCH_NAME}"
python -m tools.github_readme_sync.cli upload docs "${{ steps.preview_info.outputs.monty_version }}-${{ steps.preview_info.outputs.branch_name }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

note: Nice!

@tristanls
Copy link
Contributor

note to self: Observing documentation preview because .github/workflows/docs_preview.yml changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Changes to infrastructure triaged This issue or pull request was triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants