ci: finetune when specific workflow steps shall run #321
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces changes to multiple GitHub Actions workflow files to enhance control flow based on the pull request creator's user ID. A new job named Changes
Sequence Diagram(s)sequenceDiagram
participant PR as "Pull Request"
participant AutoReview as "Auto-Review Workflow"
participant CI as "CI Workflow"
participant Sloth as "Sloth Job"
participant Check as "should-run Job"
PR->>AutoReview: Trigger pull request event
AutoReview->>Check: Execute owner check
alt PR initiated by owner (ID 4849482)
Check-->>AutoReview: Set run=false
else
Check-->>AutoReview: Set run=true
end
AutoReview->>Review: Conditionally request review if run=true
PR->>CI: Trigger pull request event
CI->>Check: Execute owner check
alt PR initiated by owner
Check-->>CI: Set run=false
else
Check-->>CI: Set run=true
end
CI->>Sloth: Conditionally run Sloth job if run=true
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…esql-for-doctrine into workflows # Conflicts: # .github/workflows/ci.yml
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/auto-request-review.yml(1 hunks).github/workflows/ci.yml(2 hunks).github/workflows/release-please.yaml(1 hunks).github/workflows/sloth.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/release-please.yaml
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci.yml
108-108: property "continue-on-error" is not defined in object type {calculate-code-coverage: bool; doctrine-lexer: string; doctrine-orm: string; php: number}
(expression)
.github/workflows/auto-request-review.yml
33-33: property "check_owner" is not defined in object type {}
(expression)
🔇 Additional comments (7)
.github/workflows/auto-request-review.yml (1)
11-25: Clarify and Verify 'should-run' Job Logic.
The newshould-runjob sets an output based on whether the pull request originates from the repository owner. The shell command uses the expressionif [ "${{ github.event.pull_request.user.id == '4849482' }}" == "true" ]; thenwhich returns a string value. Please verify that this approach evaluates correctly in the GitHub Actions shell context and behaves as intended.
.github/workflows/ci.yml (4)
19-33: 'should-run' Job in CI: Confirm Conditional Check for Release-Please.
The addedshould-runjob checks if the pull request is from release-please (user ID'41898282') and accordingly sets the output. This logic is consistent with the intent; please double-check that the GitHub expression in the shell script and subsequent output handling work as expected.
34-38: Verify 'tests' Job Dependency and Execution Condition.
Thetestsjob now depends onshould-runand includes the conditionif: needs.should-run.outputs.run == 'true'to decide whether to run. This update aligns with the PR objectives; please confirm it meets the intended CI flow for release-please PRs.
119-127: Coverage Upload Step: Conditional Execution.
The upload to Coveralls is now conditioned on:if: matrix.calculate-code-coverage == true && github.event_name == 'pull_request'This effectively restricts coverage uploads to pull request events when enabled in the matrix. The modification appears appropriate; ensure that
matrix.calculate-code-coverageis reliably set in all intended cases.
106-108:✅ Verification successful
Review 'continue-on-error' Usage in Static Analysis Step.
The step running static analysis includes:continue-on-error: ${{ matrix.continue-on-error || false }}However, static analysis tools flag that the property
continue-on-errormay not be defined for the current object type. Verify whether this property is supported in this job context or if it should be removed or modified.
Static Analysis Step Dynamic Configuration Verified
The use of
continue-on-error: ${{ matrix.continue-on-error || false }}in the static analysis step is valid under GitHub Actions if your job is using a matrix strategy that defines the
continue-on-errorkey. GitHub Actions supports dynamic expressions for step properties, and the fallback tofalseensures proper behavior when the matrix value isn’t provided. If the static analysis tool is flagging this as unsupported, please verify that your matrix strategy includes a definition forcontinue-on-error. Otherwise, no changes are necessary.🧰 Tools
🪛 actionlint (1.7.4)
108-108: property "continue-on-error" is not defined in object type {calculate-code-coverage: bool; doctrine-lexer: string; doctrine-orm: string; php: number}
(expression)
.github/workflows/sloth.yml (2)
12-26: 'should-run' Job in Sloth Workflow: Ensure Proper Conditional Check.
Theshould-runjob in this file uses a similar conditional check for release-please by evaluating ifif [ "${{ github.event.pull_request.user.id == '41898282' }}" == "true" ]; thento set the output accordingly. This restructuring aligns with changes in the other workflows. Please verify that the user ID and logic are correct for skipping the Sloth check for the designated PRs.
27-31: 'sloth' Job Dependency and Conditional Execution.
Theslothjob now depends onshould-runand only runs whenif: needs.should-run.outputs.run == 'true'This update ensures that the Sloth job is conditionally executed, aligning the workflow with the updated control flow logic.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
112-112: 💡 Verification agent🧩 Analysis chain
Enhance the condition for uploading coverage results.
The current condition:if: matrix.calculate-code-coverage == truedoes not check whether the workflow was triggered by a pull request, even though the PR objectives indicate that the upload should occur only on pull requests. Consider updating the condition to also check that
github.event_nameequals'pull_request'. For example:- if: matrix.calculate-code-coverage == true + if: matrix.calculate-code-coverage == true && github.event_name == 'pull_request'This ensures the step runs only when both conditions are met.
Enhance Code Coverage Upload Condition
It appears that the current condition in
.github/workflows/ci.ymlonly verifies thatmatrix.calculate-code-coverageis true (line 112) without ensuring the workflow was triggered by a pull request. Since the upload of coverage results is intended only for pull requests, please update the condition accordingly. For example:- if: matrix.calculate-code-coverage == true + if: matrix.calculate-code-coverage == true && github.event_name == 'pull_request'This change ensures the step runs only when both the matrix flag is set and the event is a pull request.
🏁 Script executed:
#!/bin/bash echo "Extracting coverage condition from .github/workflows/ci.yml..." grep -R "matrix.calculate-code-coverage" .github/workflows/ci.yml || echo "No coverage condition found." echo "" echo "Looking for pull request event check in the file..." grep -R "github.event_name" .github/workflows/ci.yml || echo "No pull_request condition found."Length of output: 625
Update Upload Condition for Coverage Results
The current condition only checks if
matrix.calculate-code-coverageis true. However, it does not verify that the workflow is triggered by a pull request. Since the intent is to upload coverage results exclusively for pull requests, please update the condition as follows:- if: matrix.calculate-code-coverage == true + if: matrix.calculate-code-coverage == true && github.event_name == 'pull_request'This change ensures that the step runs only when both conditions are met.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
98-100: Review the usage ofcontinue-on-errorin the static analysis step.
The step:- name: Run static analysis run: composer run-static-analysis continue-on-error: ${{ matrix.continue-on-error || false }}raises a static analysis warning because the
matrixobject does not define acontinue-on-errorproperty. If the intention is to optionally allow this step to fail based on the matrix value, please ensure that the matrix is updated accordingly. Otherwise, you might remove the expression. For instance, if this behavior is not required, you can simply remove thecontinue-on-errorkey:- continue-on-error: ${{ matrix.continue-on-error || false }}Please verify whether you need this dynamic behavior.
🧰 Tools
🪛 actionlint (1.7.4)
100-100: property "continue-on-error" is not defined in object type {calculate-code-coverage: bool; doctrine-lexer: string; doctrine-orm: string; php: number}
(expression)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci.yml
100-100: property "continue-on-error" is not defined in object type {calculate-code-coverage: bool; doctrine-lexer: string; doctrine-orm: string; php: number}
(expression)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: PHP 8.4 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.3 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.3 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: sloth
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
19-20:✅ Verification successful
Fix the conditional check for pull request creator user ID.
The current syntaxif [ "${{ github.event.pull_request.user.id == '41898282' }}" == "true" ]; thenis problematic because it attempts to perform a boolean comparison within the GitHub Actions expression syntax inside a bash conditional. Consider comparing the user ID directly. For example:
- if [ "${{ github.event.pull_request.user.id == '41898282' }}" == "true" ]; then + if [ "${{ github.event.pull_request.user.id }}" == "41898282" ]; thenThis ensures that the shell compares the expanded pull request user ID with the expected value.
GitHub Actions Workflow Conditional Fixed
The issue has been verified. Instead of evaluating a boolean expression within the expression substitution, the workflow now directly compares the pull request creator's user ID. This ensures that the shell performs the string comparison correctly. Consider updating as follows:
File:
.github/workflows/ci.yml(Lines 19-20)Before:
if [ "${{ github.event.pull_request.user.id == '41898282' }}" == "true" ]; thenAfter:
if [ "${{ github.event.pull_request.user.id }}" == "41898282" ]; then
This reverts commit ed5ecdd.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/sloth.yml (1)
17-25: 🛠️ Refactor suggestionStandardize the Shell User ID Check
The "Skip for release-please" step contains a conditional check:
if [ "${{ github.event.pull_request.user.id == '41898282' }}" == "true" ]; thenAs with the other workflows, to avoid potential evaluation issues, it is recommended to compare the user ID directly:
- if [ "${{ github.event_pull_request.user.id == '41898282' }}" == "true" ]; then + if [ "${{ github.event_pull_request.user.id }}" == "41898282" ]; thenThis suggestion aligns with previous recommendations for improved reliability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/auto-request-review.yml(1 hunks).github/workflows/ci.yml(2 hunks).github/workflows/sloth.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci.yml
100-100: property "continue-on-error" is not defined in object type {calculate-code-coverage: bool; doctrine-lexer: string; doctrine-orm: string; php: number}
(expression)
🔇 Additional comments (7)
.github/workflows/auto-request-review.yml (3)
11-15: Output Variable Definition is ClearThe definition of the
runoutput in theshould-runjob (using${{ steps.check.outputs.run }}) is well implemented and clearly communicates the intended workflow output.
26-30: Job Dependency and Conditional Execution Look CorrectThe
auto-request-reviewjob’s dependency on theshould-runjob and its conditional execution viaif: needs.should-run.outputs.run == 'true'is implemented correctly. This ensures that the auto-review request only runs when appropriate.
17-25:✅ Verification successful
Refine the Shell Conditional Expression
In the "Skip for repository owner" step, the shell condition currently uses an inline GitHub expression for comparison:
if [ "${{ github.event.pull_request.user.id == '4849482' }}" == "true" ]; thenThis expression may not work as intended. It’s preferable to compare the evaluated user ID directly. Consider revising it as follows:
- if [ "${{ github.event.pull_request.user.id == '4849482' }}" == "true" ]; then + if [ "${{ github.event.pull_request.user.id }}" == "4849482" ]; thenThis change ensures that the shell performs a direct string comparison.
Refine the Shell Conditional for Correct User ID Comparison
The suggestion is valid. Instead of evaluating a boolean expression inline, compare the pull request user ID directly. This ensures that the shell receives the actual user ID string for accurate string comparison. Please update the conditional in the workflow as follows:
- Before:
if [ "${{ github.event.pull_request.user.id == '4849482' }}" == "true" ]; then- After:
if [ "${{ github.event.pull_request.user.id }}" == "4849482" ]; thenThis change directly compares the evaluated user ID from GitHub’s context to the expected owner ID, thus avoiding any unexpected behavior from inline boolean evaluation.
.github/workflows/ci.yml (2)
11-15: Consistent Output Handling in CI Should-Run JobThe new
should-runjob in the CI workflow defines its output consistently with the other workflows. However, please verify that downstream steps rely only on the evaluated output from the check step.
26-29: Test Job Dependency ValidatedThe
testsjob correctly depends on theshould-runjob and uses the proper condition:if: needs.should-run.outputs.run == 'true'This ensures the tests run only when the job logic permits it.
.github/workflows/sloth.yml (2)
12-16: Should-Run Job Structure is ConsistentThe
should-runjob in this workflow follows the established pattern. The use of outputs and the step structure is consistent with the other workflow files.
27-30: Dependent Sloth Job is Well ConfiguredThe newly introduced
slothjob correctly depends on theshould-runjob and its conditional execution using:if: needs.should-run.outputs.run == 'true'ensures that it runs only when intended.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/ci.yml (2)
11-25: Refined Skip Logic for Release-Please PRs
The newshould-runjob correctly determines whether CI checks should proceed based on the pull request author's user ID. The shell script uses a direct comparison withif [ "${{ github.event.pull_request.user.id }}" = '41898282' ]; thenwhich is valid. For clarity and consistency, consider using matching quotation styles (e.g., double quotes for both operands) so that it reads consistently as:
- if [ "${{ github.event.pull_request.user.id }}" = '41898282' ]; then + if [ "${{ github.event.pull_request.user.id }}" = "41898282" ]; then
112-113: Coverage Upload Condition Consideration
The step to upload coverage results uses the condition:if: matrix.calculate-code-coverage == trueSince this workflow is exclusively triggered on pull requests (lines 3–5), the additional check for
github.event_name == 'pull_request'may be superfluous. Please verify that this meets your intentions and future use cases..github/workflows/sloth.yml (1)
12-26: Consistent Skip Logic in Sloth Workflow
Theshould-runjob in this file mirrors the CI workflow by evaluating the pull request author’s user ID. The shell condition:if [ "${{ github.event.pull_request.user.id }}" = '41898282' ]; thenworks as expected. As noted for the CI workflow, aligning the quotation (e.g., using double quotes for both sides) would enhance readability and maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/auto-request-review.yml(1 hunks).github/workflows/ci.yml(2 hunks).github/workflows/sloth.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/auto-request-review.yml
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci.yml
100-100: property "continue-on-error" is not defined in object type {calculate-code-coverage: bool; doctrine-lexer: string; doctrine-orm: string; php: number}
(expression)
🔇 Additional comments (2)
.github/workflows/ci.yml (1)
26-30: Effective Dependency Setup in the Tests Job
Thetestsjob now depends on the output of theshould-runjob by checking:if: needs.should-run.outputs.run == 'true'This ensures that tests run only when the output is set to true. The design meets the PR objective for finetuning workflow steps.
.github/workflows/sloth.yml (1)
27-30: Appropriate Conditional Execution of the Sloth Job
Introducing theslothjob with a dependency onshould-runand the condition:if: needs.should-run.outputs.run == 'true'ensures that the Sloth check runs only when appropriate. This design aligns well with the PR objectives to conditionally execute workflow steps.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/ci.yml (3)
11-26: New "should-run" Job Implementation
The newly addedshould-runjob effectively determines whether subsequent jobs should execute based on the pull request author's user ID. The shell script in this block (especially line 19) now uses a direct string comparison:if [ "${{ github.event_pull_request.user.id }}" = '41898282' ]; thenThis update addresses previous concerns regarding the unreliable evaluation of boolean expressions. For future maintainability, consider adding an inline comment or documentation explaining why this specific user ID is used.
100-102: Matrix Property "continue-on-error" Consideration
The "Run static analysis" step includes the line:continue-on-error: ${{ matrix.continue-on-error || false }}However, the static analysis hint indicates that the matrix does not define a
continue-on-errorproperty (the matrix only includes properties likecalculate-code-coverage,doctrine-lexer,doctrine-orm, andphp). To avoid potential runtime issues or confusion, consider definingcontinue-on-errorexplicitly in the matrix configuration (if it’s intended to be used) or remove this reference if not necessary.🧰 Tools
🪛 actionlint (1.7.4)
101-101: property "continue-on-error" is not defined in object type {calculate-code-coverage: bool; doctrine-lexer: string; doctrine-orm: string; php: number}
(expression)
112-119: Coverage Upload Condition Clarification
The condition for the "Upload coverage results to Coveralls" step is set as:if: matrix.calculate-code-coverage == trueSince the workflow trigger is already scoped to pull requests (as defined in the
on:section), the omission of an explicitgithub.event_name == 'pull_request'check is acceptable. Nonetheless, adding a brief comment to note that the trigger condition is inherent to pull request events could improve clarity for future readers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/auto-request-review.yml(1 hunks).github/workflows/ci.yml(2 hunks).github/workflows/sloth.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/auto-request-review.yml
- .github/workflows/sloth.yml
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci.yml
101-101: property "continue-on-error" is not defined in object type {calculate-code-coverage: bool; doctrine-lexer: string; doctrine-orm: string; php: number}
(expression)
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
27-30: Correct Dependency and Conditional Trigger for the Tests Job
Thetestsjob now explicitly depends on theshould-runjob and is conditionally executed only ifneeds.should-run.outputs.runequals'true'. This dependency ensures that, when the pull request is from the designated user, the CI checks are appropriately skipped.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
100-101: Usage ofcontinue-on-erroron the Static Analysis Step
The static analysis step uses the propertycontinue-on-error: ${{ matrix.continue-on-error || false }}. However, static analysis tools indicate that thecontinue-on-errorproperty is not defined in the matrix schema. Please verify that the matrix indeed provides a value forcontinue-on-erroror consider using a fixed default value if the property is not intended to vary by matrix entry.🧰 Tools
🪛 actionlint (1.7.4)
101-101: property "continue-on-error" is not defined in object type {calculate-code-coverage: bool; doctrine-lexer: string; doctrine-orm: string; php: number}
(expression)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/auto-request-review.yml(1 hunks).github/workflows/ci.yml(2 hunks).github/workflows/sloth.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/auto-request-review.yml
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci.yml
101-101: property "continue-on-error" is not defined in object type {calculate-code-coverage: bool; doctrine-lexer: string; doctrine-orm: string; php: number}
(expression)
🔇 Additional comments (7)
.github/workflows/ci.yml (3)
11-15: Jobshould-runDeclaration and Output Assignment
The job is correctly defined with Ubuntu as the runner, and the output variablerunis derived from thecheckstep. This pattern is clear and consistent.
16-26: Conditional Logic in theSkip for release-pleaseStep
The shell script correctly compares the pull request’s user id using the"="operator to decide whether to skip CI checks. The notice messages and output settings (run=falseorrun=true) are clearly implemented. Consider verifying that the event always contains a valid user id to avoid potential edge cases.
27-30: Tests Job Dependency Condition
The tests job now depends onshould-runusing the conditionif: needs.should-run.outputs.run == 'true'. This ensures that tests will run only when the output from theshould-runjob istrue..github/workflows/sloth.yml (4)
12-16: Jobshould-runin Sloth Workflow
Theshould-runjob is correctly defined with the expected output variablerunderived from thecheckstep. The configuration mirrors the logic applied in the CI workflow, ensuring consistency across workflows.
17-26: Conditional Step "Skip for release-please" in Sloth Workflow
The shell script here effectively checks if the pull request creator’s user id equals"41898282", then setsrun=falseand logs a notice that the Sloth check will be skipped. Otherwise, it setsrun=truewith an appropriate notice. The logic is clear and matches its counterpart in the CI workflow.
28-32: JobslothDependency onshould-run
The newslothjob correctly usesneeds: should-runand includes the conditionif: needs.should-run.outputs.run == 'true'to ensure it executes only when appropriate. This fulfills the objective of conditional execution based on the PR author.
33-42: Sloth Execution Step
The step that runs the Sloth check, usinglendable/sloth@e1fd9a2df2549f6e64188f274bc5d3b39d7842ed(v0.2.0), is correctly placed under the conditional job and appears configured properly with all necessary inputs.
| - name: Upload coverage results to Coveralls | ||
| if: matrix.calculate-code-coverage == true | ||
| uses: coverallsapp/github-action@648a8eb78e6d50909eff900e4ec85cab4524a45b # v2 |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Condition for Uploading Coverage Results
The condition used here is if: matrix.calculate-code-coverage == true. According to the PR objectives, the condition was updated to include a check that the event triggering the workflow is a pull_request. If this was intended, please update the condition as follows:
- if: matrix.calculate-code-coverage == true
+ if: matrix.calculate-code-coverage == true && github.event_name == 'pull_request'This ensures that coverage results are only uploaded when the workflow is triggered by a pull request.
Action Required: Update CI Workflow Condition for Coveralls Upload
The current condition only checks matrix.calculate-code-coverage == true. However, based on the PR objectives, this condition should also verify that the workflow is triggered by a pull request. Please update the condition as follows:
- if: matrix.calculate-code-coverage == true
+ if: matrix.calculate-code-coverage == true && github.event_name == 'pull_request'This change will ensure that coverage results are uploaded only when the workflow is triggered by a pull request.
📝 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: Upload coverage results to Coveralls | |
| if: matrix.calculate-code-coverage == true | |
| uses: coverallsapp/github-action@648a8eb78e6d50909eff900e4ec85cab4524a45b # v2 | |
| - name: Upload coverage results to Coveralls | |
| - if: matrix.calculate-code-coverage == true | |
| + if: matrix.calculate-code-coverage == true && github.event_name == 'pull_request' | |
| uses: coverallsapp/github-action@648a8eb78e6d50909eff900e4ec85cab4524a45b # v2 |
Summary by CodeRabbit