kgyrtkirk commented on code in PR #18070:
URL: https://github.com/apache/druid/pull/18070#discussion_r2150490650
##########
.github/workflows/unit-and-integration-tests-unified.yml:
##########
@@ -184,33 +187,25 @@ jobs:
pull_number: context.payload.pull_request.number
});
const approved = reviews.data.some(review => review.state ===
'APPROVED');
- core.setOutput("isApproved", approved);
+ core.setOutput("runIts", approved);
- - name: Default to Approved for Branch
+ - name: Default to run ITS for master and release Branch
id: default-check
if: ${{ env.isPR == 'false' }}
- run: echo "::set-output name=isApproved::true"
+ run: echo "::set-output name=runIts::true"
- unit-tests-unapproved:
- name: "unit tests - PR unapproved"
+ unit-tests:
+ name: "unit tests"
uses: ./.github/workflows/ci.yml
- needs: [check-approval]
- if: ${{ needs.check-approval.outputs.approved != 'true' }}
-
- unit-tests-approved:
- name: "unit tests - PR approved"
- uses: ./.github/workflows/ci.yml
- needs: [check-approval]
- if: ${{ needs.check-approval.outputs.approved == 'true' }}
standard-its:
- needs: [build, unit-tests-unapproved, check-approval]
- if: ${{ !cancelled() && (needs.check-approval.outputs.approved == 'true'
|| needs.unit-tests-unapproved.result == 'success') }}
+ needs: [unit-tests, post-build-check]
Review Comment:
note: this job `needs` the `unit-tests` job - so it will be never executed
in parallel to that
in its current form it seems to me that feature was lost and it may enable
the job to skip the ITs entirely - and get a green :heavy_check_mark:
`runITs` is set when
* `isPR` is true and APPROVED
* `isPR` is false
this implicitly means that if `isPR` is true - but its not `APPROVED` it
won't run the ITs -> which mean it will get a green :heavy_check_mark: - if
someone approves the PR after that the :heavy_check_mark: remains - and someone
may merge a change which breaks ITs
the previous layout was a hack to achieve that ITs have started earlier when
the PR was approved:
GHA needs requires *all dependant* jobs to be successfull
https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#jobsjob_idneeds
the `unit-tests-unapproved` was a "quick" empty job to run if the PR was
approved; and hence provided a way to run the ITs after that - but every time
that job was a skip `unit-tests-approved` did run the tests - that was how the
parallel execution was engineered.
I think we could probably ditch the whole logic and rebuild it later if
needed - we could squeeze out some more swiftness by continuing the migration
of things to the `ci.yaml` stuff
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
