Skip to content

[Build] Fix skipping tests when there are only documentation changes and run centos7 & windows cpp build only when there are cpp changes - #10376

Merged
codelipenghui merged 2 commits into
apache:masterfrom
lhotari:lh-fix-skip-tests
Apr 28, 2021
Merged

Conversation

@lhotari

@lhotari lhotari commented Apr 26, 2021

Copy link
Copy Markdown
Member

Fixes #10373

Motivation

Tests should be skipped for PR builds that only contain documentation changes. This is currently broken.

A new build has been introduced for building the CPP client on Windows. Make the changes in ci-cpp-build-centos7.yaml and ci-cpp-build-windows.yaml so that the builds run only when there are changes in CPP client files (or the workflow definitions).

Modifications

Refactor logic to be less error prone. Use fromJSON function to parse a number in a string as a number.

Additional Context

#10276

@lhotari

lhotari commented Apr 26, 2021

Copy link
Copy Markdown
Member Author

@michaeljmarshall @eolivelli @aahmed-se Please review

@eolivelli eolivelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@merlimat merlimat added this to the 2.8.0 milestone Apr 27, 2021
@lhotari
lhotari force-pushed the lh-fix-skip-tests branch from 52628c1 to 339a1e7 Compare April 28, 2021 04:07

@michaeljmarshall michaeljmarshall left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread .github/workflows/ci-build-macos.yaml Outdated
Comment thread .github/workflows/ci-build-macos.yaml Outdated
Comment on lines +51 to +63
# pattern syntax: https://github.com/micromatch/picomatch
all:
- '**'
docs:
- 'site2/**'
- 'deployment/**'
- '.asf.yaml'
- '*.md'
- '**/*.md'

- name: Check changed files
id: check_changes
run: echo "::set-output name=docs_only::${{ fromJSON(steps.changes.outputs.all_count) == fromJSON(steps.changes.outputs.docs_count) && fromJSON(steps.changes.outputs.docs_count) > 0 }}"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems complex, but it's necessary.

picomatch translates a pattern to a regex under the covers. combining including and excluding can lead to odd results.

It's possible to test picomatch patterns using npm on the command line, for example:

# check picomatch version from https://github.com/apache/pulsar-test-infra/blob/master/paths-filter/package-lock.json
npm install picomatch@2.2.1  
function test_picomatch_pattern() { node -e 'const pattern=process.argv[1]; const matcher=require("picomatch")(pattern, {"dot": true}); console.log(`pattern: ${pattern}`); process.argv.splice(2).forEach(arg => console.log(`${arg} -> ${matcher(arg)}`));' "$@"  }

test_picomatch_pattern '**/*.md' README.md some/path/README.md

This example shows a problem with inversion (!) in the pattern:

❯ test_picomatch_pattern '**/*.md' README.md some/path/README.md
pattern: **/*.md
README.md -> true
some/path/README.md -> true
❯ test_picomatch_pattern '!(**/*.md)' README.md some/path/README.md
pattern: !(**/*.md)
README.md -> true
some/path/README.md -> false

That is the reason why the logic for checking documentation only changes is done based on the comparison on match counts,
fromJSON(steps.changes.outputs.all_count) == fromJSON(steps.changes.outputs.docs_count) && fromJSON(steps.changes.outputs.docs_count) > 0 .

pull_request:
branches:
- master
paths-ignore: ['site2/**', 'deployment/**']

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

paths-ignore would be fine here, but it's removed to be consistent with other workflows in apache/pulsar where paths-filter action (copy of https://github.com/dorny/paths-filter) is used.

GitHub provides a paths-ignore feature out of the box.
However, GitHub's paths-ignore has some limitations:

  • GitHub's paths-ignore fails to look at previous commits. This means that the outcome depends on how often you push changes.
  • Consequently, GitHub's paths-ignore does not work for required checks.
    If you path-ignore a required check, then pull requests will block forever without being mergeable.

(source)

paths-ignore was once used in apache/pulsar (introduced by #9527), but it had to be reverted in #9606 because of the limitations.

@lhotari lhotari changed the title [Build] Fix skipping tests when there are only documentation changes [Build] Fix skipping tests when there are only documentation changes and run centos7 & windows cpp build only when there are cpp changes Apr 28, 2021
@lhotari

lhotari commented Apr 28, 2021

Copy link
Copy Markdown
Member Author

/pulsarbot run-failure-checks

@codelipenghui
codelipenghui merged commit 0984022 into apache:master Apr 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Build] Skipping tests for documentation changes isn't working in the GitHub Actions workflows

5 participants