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

Jump to bottom

Fix label removal logic for SQL engine tests #1253

Merged
merged 2 commits into from
Jun 7, 2024
Merged

Fix label removal logic for SQL engine tests #1253

merged 2 commits into from
Jun 7, 2024

Conversation

plypaul
Copy link
Contributor

@plypaul plypaul commented Jun 6, 2024

Description

This PR fixes an issue where Run Tests With Other SQL Engines label isn't getting removed when tests fail.

@cla-bot cla-bot bot added the cla:yes label Jun 6, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 6, 2024

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@plypaul plypaul added Skip Changelog Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment labels Jun 6, 2024
@plypaul plypaul temporarily deployed to DW_INTEGRATION_TESTS June 6, 2024 02:13 — with GitHub Actions Inactive
@plypaul plypaul temporarily deployed to DW_INTEGRATION_TESTS June 6, 2024 02:13 — with GitHub Actions Inactive
@plypaul plypaul temporarily deployed to DW_INTEGRATION_TESTS June 6, 2024 02:13 — with GitHub Actions Inactive
@plypaul plypaul had a problem deploying to DW_INTEGRATION_TESTS June 6, 2024 02:13 — with GitHub Actions Failure
@github-actions github-actions bot removed the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Jun 6, 2024
@plypaul plypaul added the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Jun 6, 2024
@plypaul plypaul temporarily deployed to DW_INTEGRATION_TESTS June 6, 2024 02:23 — with GitHub Actions Inactive
@plypaul plypaul temporarily deployed to DW_INTEGRATION_TESTS June 6, 2024 02:23 — with GitHub Actions Inactive
@plypaul plypaul temporarily deployed to DW_INTEGRATION_TESTS June 6, 2024 02:23 — with GitHub Actions Inactive
@plypaul plypaul temporarily deployed to DW_INTEGRATION_TESTS June 6, 2024 02:23 — with GitHub Actions Inactive
@plypaul plypaul marked this pull request as ready for review June 6, 2024 02:25
@github-actions github-actions bot removed the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Jun 6, 2024
@tlento tlento self-requested a review June 6, 2024 03:32
tlento
tlento previously requested changes Jun 6, 2024
View reviewed changes
Copy link
Contributor

@tlento tlento left a comment

Choose a reason for hiding this comment

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

This breaks our workflow_dispatch

.github/workflows/cd-sql-engine-tests.yaml
@@ -15,7 +15,7 @@ env:
jobs:
snowflake-tests:
environment: DW_INTEGRATION_TESTS
if: ${{ github.event.action != 'labeled' || github.event.label.name == 'Run Tests With Other SQL Engines' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing the first part of this check breaks all other triggers, because it requires the event to have this label name.

.github/workflows/cd-sql-engine-tests.yaml
@@ -15,7 +15,7 @@ env:
jobs:
Copy link
Contributor

Choose a reason for hiding this comment

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

[Re: line 6]

 With this change this workflow_dispatch trigger will succeed in doing nothing.

See this comment inline on Graphite.

@plypaul
Copy link
Contributor Author

plypaul commented Jun 6, 2024

I'm missing some context - what were the use cases for workflow_dispatch?

Base automatically changed from p--py312--15 to main June 6, 2024 07:00
@plypaul plypaul added the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Jun 6, 2024
@plypaul plypaul added Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment and removed Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment labels Jun 6, 2024
@plypaul plypaul added Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment and removed Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment labels Jun 6, 2024
@plypaul plypaul had a problem deploying to DW_INTEGRATION_TESTS June 6, 2024 18:46 — with GitHub Actions Failure
@plypaul plypaul added Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment and removed Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment labels Jun 6, 2024
@plypaul plypaul temporarily deployed to DW_INTEGRATION_TESTS June 6, 2024 18:48 — with GitHub Actions Inactive
@plypaul plypaul temporarily deployed to DW_INTEGRATION_TESTS June 6, 2024 18:48 — with GitHub Actions Inactive
@plypaul plypaul temporarily deployed to DW_INTEGRATION_TESTS June 6, 2024 18:48 — with GitHub Actions Inactive
@plypaul plypaul had a problem deploying to DW_INTEGRATION_TESTS June 6, 2024 18:48 — with GitHub Actions Failure
@plypaul plypaul added Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment and removed Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment labels Jun 6, 2024
@plypaul plypaul had a problem deploying to DW_INTEGRATION_TESTS June 6, 2024 18:57 — with GitHub Actions Failure
@plypaul plypaul temporarily deployed to DW_INTEGRATION_TESTS June 6, 2024 18:57 — with GitHub Actions Inactive
@plypaul plypaul temporarily deployed to DW_INTEGRATION_TESTS June 6, 2024 18:57 — with GitHub Actions Inactive
@plypaul plypaul temporarily deployed to DW_INTEGRATION_TESTS June 6, 2024 18:57 — with GitHub Actions Inactive
@github-actions github-actions bot removed the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Jun 6, 2024
@plypaul plypaul added the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Jun 6, 2024
@plypaul plypaul temporarily deployed to DW_INTEGRATION_TESTS June 6, 2024 20:45 — with GitHub Actions Inactive
@plypaul plypaul temporarily deployed to DW_INTEGRATION_TESTS June 6, 2024 20:45 — with GitHub Actions Inactive
@plypaul plypaul temporarily deployed to DW_INTEGRATION_TESTS June 6, 2024 20:45 — with GitHub Actions Inactive
@plypaul plypaul temporarily deployed to DW_INTEGRATION_TESTS June 6, 2024 20:45 — with GitHub Actions Inactive
@plypaul
Copy link
Contributor Author

plypaul commented Jun 6, 2024

Got context from @tlento that the workflow dispatched used to check if main is broken. Added a comment about it.

@plypaul plypaul requested a review from tlento June 6, 2024 21:01
@github-actions github-actions bot removed the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Jun 6, 2024
@tlento tlento dismissed their stale review June 6, 2024 22:59

because

tlento
tlento approved these changes Jun 6, 2024
View reviewed changes
Copy link
Contributor

@tlento tlento left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

.github/workflows/cd-sql-engine-tests.yaml
@@ -115,7 +118,8 @@ jobs:
name: Remove Label After Running Tests
runs-on: ubuntu-latest
needs: [ snowflake-tests, redshift-tests, bigquery-tests, databricks-tests ]
if: github.event.action == 'labeled' && github.event.label.name == 'Run Tests With Other SQL Engines'
# Default behavior for `needs` is that it requires success, so a success / failure needs to be specifically checked.
if: ${{ (success() || failure()) && github.event.action == 'labeled' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh nice.

@plypaul plypaul merged commit 1d9f673 into main Jun 7, 2024
29 checks passed
@plypaul plypaul deleted the p--py312--16 branch June 7, 2024 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tlento tlento tlento approved these changes

Assignees
No one assigned
Labels
cla:yes Skip Changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@plypaul @tlento