-
Notifications
You must be signed in to change notification settings - Fork 21
Update GitHub Actions workflows (sync with other repos in org) #225
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates GitHub Actions workflows to synchronize with changes in other repositories and to upgrade Python from 3.9 to 3.10 in relevant jobs. Key changes include:
- Upgrading action versions such as actions/checkout, actions/setup-python, and related tools.
- Replacing the dedicated Poetry installation action with pipx-based commands.
- Adding pull_request triggers for documentation build and minor adjustments in branch checkout commands.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/validate-yaml-syntax.yaml | Upgraded checkout action version to v4.2.2. |
| .github/workflows/release.yaml | Upgraded checkout, setup-python, pypi-publish, and semver action versions; switched Poetry install method. |
| .github/workflows/main.yaml | Adjusted for matrix-driven Python versions, upgraded checkout and setup-python actions, and refined dependency install options. |
| .github/workflows/codespell.yml | Upgraded checkout and codespell action versions. |
| .github/workflows/build-publish-documentation.yaml | Added pull_request event trigger, input matrix for Python 3.10, updated Poetry installation, and refined git branch handling. |
| python-version: 3.9 | ||
| python-version: 3.10 | ||
|
|
||
| - name: Install Poetry |
Copilot
AI
May 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change from using snok/install-poetry to a pipx-based installation command is noted. Please add a comment explaining the rationale to ensure consistency and help future maintainers understand the decision.
| - name: Install Poetry | |
| - name: Install Poetry | |
| # Using pipx to install Poetry ensures that it is installed in an isolated environment, | |
| # avoiding potential conflicts with other Python packages. This replaces the previous | |
| # use of snok/install-poetry for better consistency and maintainability. |
| # install dependencies if cache does not exist | ||
| # install dependencies | ||
| #---------------------------------------------- | ||
| - name: Install dependencies |
Copilot
AI
May 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verify that using both '--with docs' and '--all-extras' is intentional to avoid installing unnecessary dependencies. A brief comment could clarify the intended behavior for future reviewers.
| - name: Install dependencies | |
| - name: Install dependencies | |
| # Install dependencies with both '--with docs' and '--all-extras' to ensure | |
| # that all optional dependencies, including those required for documentation, | |
| # are installed. This is intentional to support the full functionality of the workflow. |
| virtualenvs-in-project: true | ||
| run: | | ||
| pipx install poetry | ||
| pipx inject poetry poetry-dynamic-versioning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repo is not yet setup for dynamic-versioning so you don't need to install it.
| virtualenvs-in-project: true | ||
| run: | | ||
| pipx install poetry | ||
| pipx inject poetry poetry-dynamic-versioning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repo is not yet setup for dynamic-versioning so you don't need to install it.
| virtualenvs-in-project: true | ||
| run: | | ||
| pipx install poetry | ||
| pipx inject poetry poetry-dynamic-versioning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again. The repo is not yet setup for dynamic-versioning so you don't need to install it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this was a particular design decision to be honest. It could be that linkml-model repo was created before we started using dynamic versioning and we didn't bring it back up to "standard."
We can do dynamic versioning in another PR; I guess my initial reaction is to keep it consistent with other linkml processes for pypi releasing -- e.g. also add trusted publishing, etc..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also suggest to add dynamic versioning in another PR.
| # switch to gh-pages branch, copy over the latest 404.html from main | ||
| git checkout --force gh-pages | ||
| git checkout --force main -- 404.html | ||
| git checkout --force origin/main -- 404.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use mike for multi-versioning in nfdi4cat/pid4cat-model and the checkout acrobatics and the copying of the 404.html here are strange. I added a proper 404.html to the gh-pages branch and never hat to touch it again.
However, if it works here then maybe leave it as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm responsible for the checkout acrobatics here! 😂
I'm afraid I don't quite remember all the details on why it is the way it is. There is a brief comment in #168 about it. But ultimately I think I just didn't like the idea of a file only living in the gh-pages branch where it could be potentially forgotten about.
| strategy: | ||
| matrix: | ||
| python-version: [ "3.10" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason to use a matrix here when we only will ever do this using one Python version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I saw this pattern in workflows in other repos like linkml – https://github.com/linkml/linkml/blob/main/.github/workflows/doc-pages.yaml#L10-L11 and I thought that we should change it, but left it this way to be consistent with other repos.
But yes you're absolutely right, we're going to be building and publishing the docs using only one version of Python and there's no reason to have a python-version matrix like we have above for that.
I'll go ahead and make the change to correct that. Will make PR's to fix this in other repos as well.
| - name: Install dependencies | ||
| if: steps.cached-poetry-dependencies.outputs.cache-hit != 'true' | ||
| run: poetry install --no-interaction --no-root | ||
| run: poetry install --no-interaction --with docs --no-root --all-extras |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell pyproject.toml for this project doesn't specify any extras and the docs group is not marked as optional. So the --with docs and --all-extras flags aren't doing anything here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! thanks for picking up on that Patrick. Will correct it.
| # switch to gh-pages branch, copy over the latest 404.html from main | ||
| git checkout --force gh-pages | ||
| git checkout --force main -- 404.html | ||
| git checkout --force origin/main -- 404.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm responsible for the checkout acrobatics here! 😂
I'm afraid I don't quite remember all the details on why it is the way it is. There is a brief comment in #168 about it. But ultimately I think I just didn't like the idea of a file only living in the gh-pages branch where it could be potentially forgotten about.
Fixes linkml/linkml#2720
Note: Updated the docs building/publishing workflow to use Python 3.10 to build the docs instead of Python 3.9.