-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Remove separate docs GitHub workflow #27070
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
This comment was marked as outdated.
This comment was marked as outdated.
f53b204 to
223d18b
Compare
2edda65 to
9f43511
Compare
|
The workflow changes were tested 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.
Will build the docs module if the dependent Java module changes? Specifically, if you add another reserved word for the parser, does the ReservedIdentifiers check still run?
testing/trino-tests/src/test/java/io/trino/tests/ci/TestCiWorkflow.java
Outdated
Show resolved
Hide resolved
testing/trino-tests/src/test/java/io/trino/tests/ci/TestCiWorkflow.java
Outdated
Show resolved
Hide resolved
testing/trino-tests/src/test/java/io/trino/tests/ci/TestCiWorkflow.java
Outdated
Show resolved
Hide resolved
Yes
Before and after the changes, this is covered by |
Because it's the last one to run.
`mvn validate` does not trigger dependency build. It should not run any tasks that require dependencies to exist.
This avoids producing empty `gib-impacted.log` file for docs-only changes. The empty `gib-impacted.log` is also produced on `master` when GIB does not run, and is treated as indicative of "run everything".
Before the change, we had two workflows: `ci` and `docs`. - for code changes we run `ci` - for docs changes we run `docs` - for code and docs changes we run both, but the docs PR was configured to do nothing `ci` has `build-success` status marker and it's required job for a PR to be merged. We could add similar job to `docs` workflow. - it can also be called `build-success`. This would work for docs-only PRs, but not for code & docs PRs. - it can be called `docs-build-success` and also become required, but that doesn't solve original problem at all. Solving this requires that - the workflows to be truly mutually exclusive (for now they are logically mutually exclusive but not really) OR - there is only one workflow, with `docs` workflow merging into `ci` workflow. This change implements the latter option, as it's conceptually simpler and so should be better from long term maintainability perspective. - The `docs` workflow is removed. - The `ci` workflow is run on all PRs. - The `docs-checks` is already covered by `maven-checks` job in `ci` workflow. The removed `docs` workflow had one more job: `test-docs`. This was supposed to run Java tests that check docs changes. This is currently lost. However, it wasn't working reliably anyway. It would run on docs-only changes, but would not run on PRs mixing docs changes with any other code changes, unless those changes would trigger these tests to run. This is left to be fixed later.
212e26e to
a0d9b10
Compare
Before the change, we had two workflows:
cianddocs.cidocsto do nothing
cihasbuild-successstatus marker and it's required job for a PR tobe merged.
We could add similar job to
docsworkflow.build-success. This would work for docs-onlyPRs, but not for code & docs PRs.
docs-build-successand also become required, butthat doesn't solve original problem at all.
Solving this requires that
logically mutually exclusive but not really) OR
docsworkflow merging intociworkflow.
This change implements the latter option, as it's conceptually simpler
and so should be better from long term maintainability perspective.
docsworkflow is removed.ciworkflow is run on all PRs.docs-checksis already covered bymaven-checksjob inciworkflow.
The removed
docsworkflow had one more job:test-docs. This wassupposed to run Java tests that check docs changes. This is currently
lost. However, it wasn't working reliably anyway. It would run on
docs-only changes, but would not run on PRs mixing docs changes with any
other code changes, unless those changes would trigger these tests to
run (#27076). This is left to be fixed later.
Summary by Sourcery
Merge the separate docs GitHub workflow into the main CI workflow by introducing path-based filtering, reintroducing a final build-success job, and removing docs.yml.
Enhancements:
Build:
CI:
Tests: