Skip to content
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.

Sign up for GitHub

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

feat(ci): support opt-in workflow for pull-requests #12870

Merged
merged 8 commits into from
Oct 16, 2023
Merged

feat(ci): support opt-in workflow for pull-requests #12870

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fe6ef75
try opt-in-mode
kwannoel Oct 16, 2023
95e9862
add labels for all components
kwannoel Oct 16, 2023
6ac3cac
make sqlsmith tests label triggered
kwannoel Oct 16, 2023
54b9662
fix typos
kwannoel Oct 16, 2023
4060a9a
rename ci/enable-pr-opt-in-mode to ci/skip-ci + ban skip-ci in non-dr…
kwannoel Oct 16, 2023
d1034d1
fix parse
kwannoel Oct 16, 2023
0cce272
fix quote
kwannoel Oct 16, 2023
d7c4527
fix
kwannoel Oct 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 2 additions & 24 deletions ci/scripts/pr-fuzz-test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,6 @@ set -euo pipefail

source ci/scripts/common.sh

set +e
# Set features, depending on our workflow
# If sqlsmith files are modified, we run tests with sqlsmith enabled.
MATCHES="ci/scripts/cron-fuzz-test.sh\
\|ci/scripts/pr-fuzz-test.sh\
\|ci/scripts/run-fuzz-test.sh\
\|src/tests/sqlsmith"
NOT_MATCHES="\.md"
CHANGED=$(git diff --name-only origin/main | grep -v "$NOT_MATCHES" | grep "$MATCHES")
set -e

# NOTE(kwannoel): Disabled because there's some breakage after #12485,
# see https://github.com/risingwavelabs/risingwave/issues/12577.
Expand All @@ -23,19 +13,7 @@ set -e
export RUN_SQLSMITH_FRONTEND=0
export RUN_SQLSMITH=1
export SQLSMITH_COUNT=100

# Run e2e tests if changes to sqlsmith source files detected.
if [[ -n "$CHANGED" ]]; then
echo "--- Checking whether to run all sqlsmith tests"
echo "origin/main SHA: $(git rev-parse origin/main)"
echo "Changes to Sqlsmith source files detected:"
echo "$CHANGED"
export RUN_SQLSMITH=1
export SQLSMITH_COUNT=100
export TEST_NUM=32
echo "Enabled Sqlsmith tests."
else
export RUN_SQLSMITH=0
fi
export TEST_NUM=32
echo "Enabled Sqlsmith tests."

source ci/scripts/run-fuzz-test.sh
18 changes: 18 additions & 0 deletions ci/workflows/pull-request.yml
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Maybe sth like "skip-ci"? That might sound more straightforward than "opt-in-mode". 😄

The idea generally LGTM. (I also sometimes want to skip ci. lol) A little problem is the ci jobs form a DAG, so it might be problematic to turn on/off all steps via labels?

Copy link
Contributor Author

@kwannoel kwannoel Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

A little problem is the ci jobs form a DAG, so it might be problematic to turn on/off all steps via labels?

Primary usecase is to enable a few tests.
So I think it's fine to let the user resolve the DAG themselves.
Usually the DAG is at most 1-2 length-wise.

Copy link
Contributor Author

@kwannoel kwannoel Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

skip-ci

+1, I will change it to ci/skip-ci

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw I think this feature should only be allowed for draft PRs to avoid accidentally disabling some tests.

Copy link
Contributor

@huangjw806 huangjw806 Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Now, drft pr will automatically skip ci. What this PR wants to do is to only run the draft pr of the label. You are right, you need to figure out the dependencies of CI job in advance.

Copy link
Contributor Author

@kwannoel kwannoel Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Btw I think this feature should only be allowed for draft PRs to avoid accidentally disabling some tests.

and

+1, I will change it to ci/skip-ci

Done in ebe56d3

Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ steps:
- label: "build"
command: "ci/scripts/build.sh -p ci-dev"
key: "build"
if: (!build.pull_request.labels includes "ci/enable-pr-opt-in-mode" || build.pull_request.labels includes "ci/run-build")
plugins:
- docker-compose#v4.9.0:
run: rw-build-env
Expand All @@ -30,6 +31,7 @@ steps:
- label: "build other components"
command: "ci/scripts/build-other.sh"
key: "build-other"
if: (!build.pull_request.labels includes "ci/enable-pr-opt-in-mode" || build.pull_request.labels includes "ci/run-build-other")
plugins:
- seek-oss/aws-sm#v2.3.1:
env:
Expand All @@ -46,6 +48,7 @@ steps:
- label: "build (deterministic simulation)"
command: "ci/scripts/build-simulation.sh"
key: "build-simulation"
if: (!build.pull_request.labels includes "ci/enable-pr-opt-in-mode" || build.pull_request.labels includes "ci/run-build-simulation")
plugins:
- docker-compose#v4.9.0:
run: rw-build-env
Expand All @@ -57,6 +60,7 @@ steps:
- label: "docslt"
command: "ci/scripts/docslt.sh"
key: "docslt"
if: (!build.pull_request.labels includes "ci/enable-pr-opt-in-mode" || build.pull_request.labels includes "ci/run-docslt")
plugins:
- docker-compose#v4.9.0:
run: rw-build-env
Expand All @@ -67,6 +71,7 @@ steps:

- label: "end-to-end test"
command: "ci/scripts/e2e-test.sh -p ci-dev -m ci-3streaming-2serving-3fe"
if: (!build.pull_request.labels includes "ci/enable-pr-opt-in-mode" || build.pull_request.labels includes "ci/run-e2e-test")
depends_on:
- "build"
- "build-other"
Expand All @@ -82,6 +87,7 @@ steps:

- label: "end-to-end test (parallel)"
command: "ci/scripts/e2e-test-parallel.sh -p ci-dev"
if: (!build.pull_request.labels includes "ci/enable-pr-opt-in-mode" || build.pull_request.labels includes "ci/run-e2e-parallel-tests")
depends_on:
- "build"
- "docslt"
Expand Down Expand Up @@ -124,6 +130,7 @@ steps:

- label: "end-to-end source test"
command: "ci/scripts/e2e-source-test.sh -p ci-dev"
if: (!build.pull_request.labels includes "ci/enable-pr-opt-in-mode" || build.pull_request.labels includes "ci/run-e2e-source-tests")
depends_on:
- "build"
- "build-other"
Expand All @@ -138,6 +145,7 @@ steps:

- label: "end-to-end sink test"
command: "ci/scripts/e2e-sink-test.sh -p ci-dev"
if: (!build.pull_request.labels includes "ci/enable-pr-opt-in-mode" || build.pull_request.labels includes "ci/run-e2e-sink-tests")
depends_on:
- "build"
- "build-other"
Expand Down Expand Up @@ -248,6 +256,7 @@ steps:

- label: "regress test"
command: "ci/scripts/regress-test.sh -p ci-dev"
if: (!build.pull_request.labels includes "ci/enable-pr-opt-in-mode" || build.pull_request.labels includes "ci/run-regress-test")
depends_on: "build"
plugins:
- docker-compose#v4.9.0:
Expand All @@ -263,6 +272,7 @@ steps:
# This ensures our `main-cron` workflow will be stable.
- label: "unit test"
command: "ci/scripts/pr-unit-test.sh"
if: (!build.pull_request.labels includes "ci/enable-pr-opt-in-mode" || build.pull_request.labels includes "ci/run-unit-test")
plugins:
- ./ci/plugins/swapfile
- seek-oss/aws-sm#v2.3.1:
Expand All @@ -278,6 +288,7 @@ steps:

- label: "check"
command: "ci/scripts/check.sh"
if: (!build.pull_request.labels includes "ci/enable-pr-opt-in-mode" || build.pull_request.labels includes "ci/run-check")
plugins:
- gencer/cache#v2.4.10:
id: cache
Expand All @@ -299,6 +310,7 @@ steps:

- label: "unit test (deterministic simulation)"
command: "ci/scripts/deterministic-unit-test.sh"
if: (!build.pull_request.labels includes "ci/enable-pr-opt-in-mode" || build.pull_request.labels includes "ci/run-unit-test-deterministic-simulation")
plugins:
- docker-compose#v4.9.0:
run: rw-build-env
Expand All @@ -310,6 +322,7 @@ steps:

- label: "integration test (deterministic simulation)"
command: "TEST_NUM=5 ci/scripts/deterministic-it-test.sh"
if: (!build.pull_request.labels includes "ci/enable-pr-opt-in-mode" || build.pull_request.labels includes "ci/run-integration-test-deterministic-simulation")
depends_on: "build-simulation"
plugins:
- docker-compose#v4.9.0:
Expand All @@ -321,6 +334,7 @@ steps:

- label: "end-to-end test (deterministic simulation)"
command: "TEST_NUM=16 ci/scripts/deterministic-e2e-test.sh"
if: (!build.pull_request.labels includes "ci/enable-pr-opt-in-mode" || build.pull_request.labels includes "ci/run-e2e-test-deterministic-simulation")
depends_on: "build-simulation"
plugins:
- seek-oss/aws-sm#v2.3.1:
Expand All @@ -339,6 +353,7 @@ steps:

- label: "recovery test (deterministic simulation)"
command: "TEST_NUM=8 KILL_RATE=0.5 ci/scripts/deterministic-recovery-test.sh"
if: (!build.pull_request.labels includes "ci/enable-pr-opt-in-mode" || build.pull_request.labels includes "ci/run-recovery-test-deterministic-simulation")
depends_on: "build-simulation"
plugins:
# - seek-oss/aws-sm#v2.3.1:
Expand All @@ -358,6 +373,7 @@ steps:

- label: "misc check"
command: "ci/scripts/misc-check.sh"
if: (!build.pull_request.labels includes "ci/enable-pr-opt-in-mode" || build.pull_request.labels includes "ci/run-misc-check")
plugins:
- docker-compose#v4.9.0:
run: rw-build-env
Expand Down Expand Up @@ -492,8 +508,10 @@ steps:
timeout_in_minutes: 30
retry: *auto-retry

# FIXME(kwannoel): Let the github PR labeller label it, if sqlsmith source files has changes.
- label: "fuzz test"
command: "ci/scripts/pr-fuzz-test.sh -p ci-dev"
if: build.pull_request.labels includes "ci/run-sqlsmith-fuzzing-tests"
depends_on:
- "build"
- "build-simulation"
Expand Down
Loading