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.

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

ci: always execute tests on non-PRs #13260

Merged
merged 1 commit into from Feb 22, 2021

Conversation

villebro
Copy link
Member

@villebro villebro commented Feb 21, 2021

SUMMARY

When working on a branch before opening a PR I noticed that the conditional test skipping doesn't work properly when PR_NUMBER isn't available (=not a PR). This changes tests to always execute when it isn't a PR, for example after merging or when just working on a branch.

BEFORE

image

AFTER

image

TEST PLAN

Local testing + ShellCheck:
image

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@ktmud
Copy link
Member

ktmud commented Feb 21, 2021

I wonder whether it'd make the workflow scripts easier to understand if we use step output instead of outcome , considering all future steps will have to check the step results anyway.

for FILE in ${FILES}
do
  for REGEX in "${REGEXES[@]}"
  do
    if [[ "${FILE}" =~ ${REGEX} ]]; then
      echo "Detected changes in following file: ${FILE}"
      exit 0
    fi
  done
done
echo "No matching file changes detected"
echo "::set-output name=no_matching_change::true"

Then maybe we can also use condition check on the check job itself (if: github.event_name == 'pull_request') to address what this PR is trying to solve? I.e. run the file change check only on pull requests, and skip future tasks only if steps.check_file_change.output.no_matching_change = true.

Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

Tested this change on a branch and it works!

@villebro
Copy link
Member Author

I wonder whether it'd make the workflow scripts easier to understand if we use step output instead of outcome , considering all future steps will have to check the step results anyway.

for FILE in ${FILES}
do
  for REGEX in "${REGEXES[@]}"
  do
    if [[ "${FILE}" =~ ${REGEX} ]]; then
      echo "Detected changes in following file: ${FILE}"
      exit 0
    fi
  done
done
echo "No matching file changes detected"
echo "::set-output name=no_matching_change::true"

Then maybe we can also use condition check on the check job itself (if: github.event_name == 'pull_request') to address what this PR is trying to solve? I.e. run the file change check only on pull requests, and skip future tasks only if steps.check_file_change.output.no_matching_change = true.

This is a really good idea; I'll merge this one first to fix the problem but will follow up with something along these lines if it works well.

@villebro villebro merged commit 15567dd into apache:master Feb 22, 2021
@villebro villebro deleted the villebro/ci-non-pr branch February 22, 2021 06:23
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io size/XS 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants