A better solution for skipping CI steps for docs-only changes. #16053
Conversation
benjyw
force-pushed
the
better_docs_only_ci_fix
branch
from
38cdd48 to
ae2f071
Compare
| @@ -5,7 +5,7 @@ | |||
|
|
|||
| jobs: | |||
| audit: | |||
| if: ${{ github.repository_owner == 'pantsbuild' }} | |||
| if: github.repository_owner == 'pantsbuild' | |||
There was a problem hiding this comment.
The ${{ }} are unnecessary in an if - GHA always evaluates the contents of if as an expression - and removing them makes it easier to add other conditions later by string manipulation.
| if: (${{ github.repository_owner == 'pantsbuild' }}) && needs.docs_only_check.outputs.docs_only | ||
| == 0 | ||
| if: (github.repository_owner == 'pantsbuild') && (needs.docs_only_check.outputs.docs_only | ||
| != 'DOCS_ONLY') |
There was a problem hiding this comment.
With this new change, we only set the output at all if this is in fact a docs-only change. In other cases we get an empty string. So rather than setting it to "1" or "true", or something that would imply that "0" or "false" are also possible values, we set it to some sentinel string, and we check the inverse with != <the value> instead of == <the inverse value>
| with: | ||
| format: json | ||
| files_ignore: docs/** | ||
| files_ignore_separator: '|' |
There was a problem hiding this comment.
This is future-proofing, in case we have other paths to ignore (see the generator script for details).
| - id: docs_only_check | ||
| if: steps.files.outputs.any_changed != 'true' |
There was a problem hiding this comment.
This is how the marketplace action indicates that no files in the set we care about (everything not under docs/) were changed
| }, | ||
| ] | ||
| ) | ||
| if get_commit_msg: |
There was a problem hiding this comment.
The job now requires us to do the checkout, but it doesn't care about the commit message, so we allow fetching it to be skipped.
benjyw
force-pushed
the
better_docs_only_ci_fix
branch
from
ae2f071 to
9c03af1
Compare
|
|
||
| def is_docs_only() -> Jobs: | ||
| """Check if this change only involves docs.""" | ||
| linux_x86_64_helper = Helper(Platform.LINUX_X86_64) | ||
| docs_files = ["docs/**"] |
There was a problem hiding this comment.
Would it make sense to circle back now that the action can handle something like **/*.md to add this generic pattern to catch / encourage markdown in the source tree along with markdown edits?
There was a problem hiding this comment.
Yeah, I think that could make a lot of sense! Probably after we move off readme.com, since their syncing logic expects a certain directory structure. But if we switch to something where we get to control the layout mapping, we can probably have the best of both worlds.
There was a problem hiding this comment.
I was actually thinking its good and fine to have docs that are not even part of a doc site. Generally notes to people who actually hack on Pants itself.
There was a problem hiding this comment.
That does make sense!
…build#16053) The previous solution involved some complexity in extracting the filenames from JSON, and was not robust to spaces in path names (we do have such paths under docs/). Instead we use a different GHA marketplace action. This one is more featureful, and allows us to specify a subset of files to look at, so we don't need to do the filtering ourselves. We can just check if the filtered set (everything except docs/**) has any changes in it.
| @@ -897,7 +891,7 @@ def merge_ok(needs: list[str], docs_only: bool) -> Jobs: | |||
| def generate() -> dict[Path, str]: | |||
| """Generate all YAML configs with repo-relative paths.""" | |||
|
|
|||
| docs_only_cond = "needs.docs_only_check.outputs.docs_only == 0" | |||
| not_docs_only = _docs_only_cond(docs_only=False) | |||
| pr_jobs = test_workflow_jobs([PYTHON37_VERSION], cron=False) | |||
| pr_jobs.update(**is_docs_only()) | |||
| for key, val in pr_jobs.items(): | |||
There was a problem hiding this comment.
Looks like it is nearly time for Job to become a proper class.
The previous solution involved some complexity in extracting the filenames
from JSON, and was not robust to spaces in path names (we do have such
paths under docs/).
Instead we use a different GHA marketplace action. This one is more featureful, and
allows us to specify a subset of files to look at, so we don't need to do the filtering
ourselves. We can just check if the filtered set (everything except docs/**)
has any changes in it.