Ci test docs #504
Ci test docs #504
Conversation
WalkthroughThis update introduces a GitHub Actions labeler workflow and configuration to automatically label pull requests that modify documentation files. It also adds a conditional documentation build-and-publish job to the test workflow, triggered by the "documentation" label. Minor corrections to docstrings and issue template capitalization are included. Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub
participant PR
participant Labeler Action
participant Test Workflow
GitHub->>PR: PR opened or updated
PR->>Labeler Action: Trigger labeler workflow
Labeler Action->>PR: Apply "documentation" label if docs/** or *.md changed
PR->>Test Workflow: Trigger test workflow
alt PR labeled "documentation"
Test Workflow->>build-docs job: Build and publish documentation
else PR not labeled "documentation"
Test Workflow->>test job: Run standard tests only
end
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
.github/workflows/labeler.yml (1)
13-16: Be explicit about the config path to avoid future ambiguityConsider passing
configuration-path: ".github/labeler.yml"to the action.
If the default lookup ever changes (or the file is relocated) the job will still work.- uses: actions/labeler@v5 with: repo-token: "${{ secrets.GITHUB_TOKEN }}" + configuration-path: ".github/labeler.yml".github/ISSUE_TEMPLATE/bug_report.md (1)
24-28: Fix list-item indentation flagged by markdownlint
MD007warns because the bullet at line 26 is indented one space deeper than the others.- OS [e.g. Ubuntu 20.04]: - - DASCore Version [e.g. 0.0.5]: + - DASCore Version [e.g. 0.0.5]: - Python Version [e.g. 3.10]:.github/workflows/runtests.yml (2)
69-70: Clean up trailing whitespace
yamllintis already flagging these two blank-lines because they contain stray spaces. While GitHub Actions’ runner will happily ignore them, they keep the file from passing basic linting in many editors/CI linters.- # Tests building the docs if the PR is labeled as documentation + # Tests building the docs if the PR is labeled as documentation(…and delete the spaces on the previous blank line as well.)
75-76: Indentation triggers ayamllinterror
yamllintreportsexpected 4 but found 6for the first list item understeps:.
YAML tolerates either style, but mixing the two (4-space vs 2-space indent for list items) in the same file tends to bite later when someone inserts anchors or overrides.Pick one style and keep it consistent, e.g.
- steps: - - uses: actions/checkout@v4 + steps: + - uses: actions/checkout@v4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/ISSUE_TEMPLATE/bug_report.md(1 hunks).github/labeler.yml(1 hunks).github/workflows/labeler.yml(1 hunks).github/workflows/runtests.yml(1 hunks)dascore/transform/dispersion.py(1 hunks)dascore/transform/taup.py(2 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
.github/ISSUE_TEMPLATE/bug_report.md
26-26: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
🪛 GitHub Actions: TestCodeMinDeps
.github/labeler.yml
[error] 1-1: Git error: invalid path '.github/labeler.yml '
🪛 GitHub Actions: TestCode
.github/labeler.yml
[error] 1-1: Git error: invalid path detected for the file '.github/labeler.yml '
🪛 GitHub Actions: LintCode
.github/labeler.yml
[error] 1-1: Pre-commit hook 'end-of-file-fixer' failed and modified this file to fix end-of-file issues.
🪛 GitHub Actions: Label PRs
.github/labeler.yml
[warning] 1-1: The config file was not found at .github/labeler.yml. Make sure it exists and that this action has the correct access rights.
🪛 YAMLlint (1.37.1)
.github/workflows/runtests.yml
[error] 69-69: trailing spaces
(trailing-spaces)
[error] 70-70: trailing spaces
(trailing-spaces)
[warning] 76-76: wrong indentation: expected 4 but found 6
(indentation)
[error] 94-94: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
dascore/transform/taup.py (1)
101-103: Docstring header fix looks goodChanging “Examples / --------” to “Example / -------” aligns the underline length with the header. No action needed.
dascore/transform/dispersion.py (1)
85-87: Typo in example variable name corrected
mirrored_patchreplaces the garbled identifier – makes the example runnable. Nice catch..github/workflows/runtests.yml (1)
80-84: Verify thatmamba-install-dascoresupports theenvironment-fileinputThe previous job only passes
python-version; here we addwith: python-version: "3.12" environment-file: './.github/doc_environment.yml'If the custom composite action wasn’t updated to accept
environment-file, the job will silently ignore the argument and use the defaultenvironment.yml, causing divergent doc builds or outright failures.Please confirm (or update the action).
.github/labeler.yml
Outdated
| documentation: | ||
| - 'docs/**' | ||
| - '**/*.md' No newline at end of file |
There was a problem hiding this comment.
Fix the filename – current path contains a trailing space and breaks all CI jobs
CI reports Git error: invalid path '.github/labeler.yml ' and the labeler action cannot find the config.
You accidentally committed the file with a trailing space in its path, so it’s written as
.github/labeler.yml␠ rather than .github/labeler.yml.
- Rename the file to remove the trailing space (
git mv ".github/labeler.yml " .github/labeler.yml). - Commit with a proper newline at EOF to satisfy
end-of-file-fixer.
documentation:
- 'docs/**'
- '**/*.md'
+Without this fix, every workflow that tries to read the config will fail.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: TestCodeMinDeps
[error] 1-1: Git error: invalid path '.github/labeler.yml '
🪛 GitHub Actions: TestCode
[error] 1-1: Git error: invalid path detected for the file '.github/labeler.yml '
🪛 GitHub Actions: LintCode
[error] 1-1: Pre-commit hook 'end-of-file-fixer' failed and modified this file to fix end-of-file issues.
🪛 GitHub Actions: Label PRs
[warning] 1-1: The config file was not found at .github/labeler.yml. Make sure it exists and that this action has the correct access rights.
🤖 Prompt for AI Agents
In the file .github/labeler.yml at lines 1 to 3, the filename has a trailing
space causing CI failures. Rename the file to remove the trailing space using
git mv to correct the path to .github/labeler.yml. Then commit the change
ensuring the file ends with a proper newline to satisfy the end-of-file-fixer.
Filename contains trailing whitespace → CI cannot locate the config file
All CI jobs are failing with “invalid path '.github/labeler.yml 'and “config file not found” because the committed filename (and possibly the path in the Git index) ends with one or more space characters. Git treats that as a different pathname, while the workflows andactions/labelerlook for the exact literal.github/labeler.yml`.
Action items
- Rename the file in the repo to remove the trailing spaces:
git mv ".github/labeler.yml " ".github/labeler.yml"
- Commit with a proper newline at EOF to satisfy the
end-of-file-fixerhook.
Without fixing the path, the labeler workflow will never load this config and documentation PRs will not be auto-labeled.
🧰 Tools
🪛 GitHub Actions: TestCodeMinDeps
[error] 1-1: Git error: invalid path '.github/labeler.yml '
🪛 GitHub Actions: TestCode
[error] 1-1: Git error: invalid path detected for the file '.github/labeler.yml '
🪛 GitHub Actions: LintCode
[error] 1-1: Pre-commit hook 'end-of-file-fixer' failed and modified this file to fix end-of-file issues.
🪛 GitHub Actions: Label PRs
[warning] 1-1: The config file was not found at .github/labeler.yml. Make sure it exists and that this action has the correct access rights.
🤖 Prompt for AI Agents
In the file .github/labeler.yml at lines 1 to 3, the filename contains trailing
whitespace causing CI to fail to locate the config file. Rename the file to
remove the trailing spaces by using git mv to rename ".github/labeler.yml " to
".github/labeler.yml". Then commit the change ensuring there is a proper newline
at the end of the file to satisfy the end-of-file-fixer hook.
.github/workflows/runtests.yml
Outdated
| - name: publish docs to netlify | ||
| shell: bash -l {0} | ||
| env: | ||
| QUARTO_PRINT_STACK: true | ||
| NETLIFY_AUTH_TOKEN: ${{ secrets.NETLIFY_AUTH_TOKEN }} | ||
| run: | | ||
| quarto publish docs --no-prompt --no-browser | ||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure Quarto is available before publishing
The final step assumes quarto is on the PATH, but the preceding build-docs action does not guarantee that. If Quarto is installed inside that composite action’s environment, great; otherwise this step will 404.
Consider making the dependency explicit, e.g.
- - name: publish docs to netlify
+ - name: publish docs to Netlify
shell: bash -l {0}
env:
QUARTO_PRINT_STACK: true
NETLIFY_AUTH_TOKEN: ${{ secrets.NETLIFY_AUTH_TOKEN }}
run: |
- quarto publish docs --no-prompt --no-browser
+ which quarto || { echo "Quarto not found"; exit 1; }
+ quarto publish docs --no-prompt --no-browser📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: publish docs to netlify | |
| shell: bash -l {0} | |
| env: | |
| QUARTO_PRINT_STACK: true | |
| NETLIFY_AUTH_TOKEN: ${{ secrets.NETLIFY_AUTH_TOKEN }} | |
| run: | | |
| quarto publish docs --no-prompt --no-browser | |
| - name: publish docs to Netlify | |
| shell: bash -l {0} | |
| env: | |
| QUARTO_PRINT_STACK: true | |
| NETLIFY_AUTH_TOKEN: ${{ secrets.NETLIFY_AUTH_TOKEN }} | |
| run: | | |
| which quarto || { echo "Quarto not found"; exit 1; } | |
| quarto publish docs --no-prompt --no-browser |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 94-94: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
In .github/workflows/runtests.yml around lines 87 to 94, the publish docs step
assumes Quarto is available on the PATH, but this is not guaranteed by the
previous build-docs action. To fix this, explicitly install or ensure Quarto is
accessible before running the publish command, such as adding a step to install
Quarto or sourcing its environment so the quarto command is found.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #504 +/- ##
=======================================
Coverage 99.85% 99.85%
=======================================
Files 118 118
Lines 9711 9711
=======================================
Hits 9697 9697
Misses 14 14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Although I added "documentation" label to this PR (and then also re-ran the workflow tests), the |
.github/workflows/labeler.yml
Outdated
| pull-requests: write | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 # pulls the PR code |
There was a problem hiding this comment.
Is the checkout action needed for automatic labeling? The example on the docs page doesn't use it.
There was a problem hiding this comment.
Yeah I don't think it's needed.
.github/workflows/runtests.yml
Outdated
| - name: publish docs to netlify | ||
| shell: bash -l {0} | ||
| env: | ||
| QUARTO_PRINT_STACK: true | ||
| NETLIFY_AUTH_TOKEN: ${{ secrets.NETLIFY_AUTH_TOKEN }} | ||
| run: | | ||
| quarto publish docs --no-prompt --no-browser | ||
There was a problem hiding this comment.
We probably don't want to publish the docs until they actually merge into master. Otherwise, we might burn through our free hosting on netlify, and it could be confusing for users looking for the development docs if they are constantly changing as someone works on a PR.
Consider just uploading the built docs as an artifact using the upload action, then the users can look at them. I think its the docs/_site directory you would want to upload.
There was a problem hiding this comment.
Agreed. Just removed it.
It appears something may be broken with the action triggering the PR based on the tag. I think the comments on this SO post might be helpful. Actually, thinking more about it, I am inclined to put this in it's own action. So, I suggest making a new workflow called If this is right, it will run under two conditions: 1) when a new commit is pushed to the PR AND the PR has the |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/runtests.yml (2)
69-76: Fix YAML-lint issues (trailing spaces & wrong indent)Minor but noisy: several lines carry stray spaces and
steps:is indented with 6 instead of 4 spaces, trippingyamllint.- build-docs: - if: contains(github.event.pull_request.labels.*.name, 'documentation') - runs-on: ubuntu-latest + build-docs: + if: contains(github.event.pull_request.labels.*.name, 'documentation') + runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v4…and delete the trailing blanks on 69, 70, 86, 87.
72-73: Guard expression against non-PR eventsWhen this workflow runs on a
pushtomaster,github.event.pull_requestdoesn’t exist. The current expression usually resolves tofalse, but GitHub has thrown “Property not found” errors in the past.-if: contains(github.event.pull_request.labels.*.name, 'documentation') +if: github.event_name == 'pull_request' && + contains(github.event.pull_request.labels.*.name, 'documentation')Makes intent explicit and future-proof.
.github/workflows/test_doc_build.yml (1)
25-29: Strip trailing whitespace to appease lintersLine 29 ends with two spaces;
yamllintflags it.- retention-days: 1␠␠ + retention-days: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/labeler.yml(1 hunks).github/workflows/labeler.yml(1 hunks).github/workflows/runtests.yml(1 hunks).github/workflows/test_doc_build.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/labeler.yml
- .github/workflows/labeler.yml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/runtests.yml
[error] 69-69: trailing spaces
(trailing-spaces)
[error] 70-70: trailing spaces
(trailing-spaces)
[warning] 76-76: wrong indentation: expected 4 but found 6
(indentation)
[error] 86-86: trailing spaces
(trailing-spaces)
[error] 87-87: trailing spaces
(trailing-spaces)
.github/workflows/test_doc_build.yml
[error] 29-29: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: test_code_min_deps (windows-latest, 3.13)
- GitHub Check: test_code_min_deps (windows-latest, 3.12)
- GitHub Check: test_code_min_deps (macos-latest, 3.13)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (windows-latest, 3.10)
- GitHub Check: build-docs
- GitHub Check: test_build_docs
🔇 Additional comments (1)
.github/workflows/test_doc_build.yml (1)
1-30: Redundancy withbuild-docsjob inruntests.ymlThis standalone workflow is cleaner, but while
build-docsstill lives inruntests.ymlboth will fire, wasting CI minutes. After removing the duplicated job there, this file alone covers the use-case.
|
|
||
| # Tests building the docs if the PR is labeled as documentation | ||
| build-docs: | ||
| if: contains(github.event.pull_request.labels.*.name, 'documentation') | ||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| fetch-tags: 'true' | ||
|
|
||
| - uses: ./.github/actions/mamba-install-dascore | ||
| with: | ||
| python-version: "3.12" | ||
| environment-file: './.github/doc_environment.yml' | ||
|
|
||
| - uses: ./.github/actions/build-docs | ||
|
|
||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid duplicated documentation build logic – keep it in one workflow only
build-docs here performs the exact same work that the brand-new TestDocBuild workflow does. Duplicating the job means:
- two identical doc builds are queued for every labelled PR,
- twice the runner minutes & cache pressure,
- twice the artefacts to sift through.
Unless you still need the matrix from runtests.yml, drop this job and let the dedicated workflow own the doc-build responsibility.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 69-69: trailing spaces
(trailing-spaces)
[error] 70-70: trailing spaces
(trailing-spaces)
[warning] 76-76: wrong indentation: expected 4 but found 6
(indentation)
[error] 86-86: trailing spaces
(trailing-spaces)
[error] 87-87: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
In .github/workflows/runtests.yml between lines 69 and 87, the build-docs job
duplicates the documentation build logic already handled by the TestDocBuild
workflow. To fix this, remove the entire build-docs job from runtests.yml so
that only the dedicated TestDocBuild workflow manages documentation builds,
avoiding redundant runs and resource usage.
Yes, it makes more sense to have this under a new workflow test. Just added it. However, shouldn't we have (github.event.action == 'labeled' && contains(github.event.pull_request.labels.*.name, 'documentation'))
|| (github.event.action == 'synchronize' && contains(github.event.pull_request.labels.*.name, 'documentation')) |
ahmadtourei
added
documentation
|
I see TestDocBuild / test_build_docs is built when the PR has the "documentation" label and also when the label is added later to the PR. |
Ya, I guess this makes sense. |
|
Actually no, I just added the CI tag and it triggered the doc build. We want it to only trigger it if the |
Gotcha, thanks for clarifying. Fun little CI project :) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/test_doc_build.yml (2)
29-29: Trailing whitespace breaks YAML-lint and can fail strict CI checksLine 29 ends with one extra space. Remove it to keep the workflow YAML-lint-clean.
- retention-days: 1␠ + retention-days: 1
10-11: Minor readability: use folded scalar instead of literal block for multi-lineifIf you prefer keeping the expression multi-line, switch
|➜>so the newline is folded into a single space, avoiding an embedded\ninside the expression string:- if: | + if: >-Not required once the single-line
${{ … }}variant above is adopted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test_doc_build.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/test_doc_build.yml
[error] 29-29: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: test_code_min_deps (windows-latest, 3.12)
- GitHub Check: test_code_min_deps (macos-latest, 3.13)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
- GitHub Check: test_code_min_deps (macos-latest, 3.12)
- GitHub Check: test_code_min_deps (windows-latest, 3.13)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.10)
- GitHub Check: test_code (macos-latest, 3.12)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_build_docs
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: test_code (macos-latest, 3.11)
- GitHub Check: test_code (macos-latest, 3.10)
- GitHub Check: build-docs
| if: | | ||
| (github.event.action == 'labeled' && github.event.label.name == 'documentation') | ||
| || (github.event.action == 'synchronize' && contains(github.event.pull_request.labels.*.name, 'documentation')) | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
🛠️ Refactor suggestion
✅ Verification successful
Expression should be wrapped in ${{ … }} to guarantee evaluation & avoid silent‐string fallback
GitHub treats the if value as a literal string unless it begins with ${{.
While an unwrapped expression usually still works, you lose linter/IDE validation and can hit subtle quoting bugs (e.g. an accidental literal "false" will not be parsed). Wrapping also lets the runner short-circuit early instead of scheduling the job and then discarding it.
- if: |
- (github.event.action == 'labeled' && github.event.label.name == 'documentation')
- || (github.event.action == 'synchronize' && contains(github.event.pull_request.labels.*.name, 'documentation'))
+ if: ${{ github.event.action == 'labeled' && github.event.label.name == 'documentation'
+ || github.event.action == 'synchronize' && contains(github.event.pull_request.labels.*.name, 'documentation') }}Expression should be wrapped in ${{ … }} to guarantee evaluation & avoid silent‐string fallback
GitHub treats the if value as a literal string unless it begins with ${{.
While an unwrapped expression usually still works, you lose linter/IDE validation and can hit subtle quoting bugs (e.g. an accidental literal "false" will not be parsed). Wrapping also lets the runner short-circuit early instead of scheduling the job and then discarding it.
- if: |
- (github.event.action == 'labeled' && github.event.label.name == 'documentation')
- || (github.event.action == 'synchronize' && contains(github.event.pull_request.labels.*.name, 'documentation'))
+ if: ${{ github.event.action == 'labeled' && github.event.label.name == 'documentation'
+ || github.event.action == 'synchronize' && contains(github.event.pull_request.labels.*.name, 'documentation') }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if: | | |
| (github.event.action == 'labeled' && github.event.label.name == 'documentation') | |
| || (github.event.action == 'synchronize' && contains(github.event.pull_request.labels.*.name, 'documentation')) | |
| runs-on: ubuntu-latest | |
| if: ${{ github.event.action == 'labeled' && github.event.label.name == 'documentation' | |
| || github.event.action == 'synchronize' && contains(github.event.pull_request.labels.*.name, 'documentation') }} | |
| runs-on: ubuntu-latest |
🤖 Prompt for AI Agents
In .github/workflows/test_doc_build.yml around lines 9 to 12, the if condition
expression is not wrapped in ${{ ... }}, causing GitHub Actions to treat it as a
literal string. To fix this, wrap the entire multiline if expression inside ${{
and }} to ensure proper evaluation, enable linter/IDE validation, and prevent
subtle bugs.
|
The CI's behavior is as expected. Just tested it by adding document and non-document labels. |
Description
This PR adds GitHub workflows to build docs in PRs with the "documentation" label.
Checklist
I have (if applicable):
Summary by CodeRabbit