#427: Implemented GH approval for slow tests #428

tomuben · 2024-11-22T14:43:18Z

fixes #427

All Submissions:

Is the title of the Pull Request correct?
Is the title of the corresponding issue correct?
Have you updated the changelog?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?
Are you mentioning the issue which this PullRequest fixes ("Fixes...")
Before you merge don't forget to run all tests for all Exasol version, by adding [run all tests] to the commit message
Are the CLI usage examples up to date?

and use matrix-python.yml in tests_with_coverage.yml

tomuben · 2025-01-14T17:26:37Z

.github/workflows/matrix-python.yml

From

tbx workflow install matrix-python

without modification

tomuben · 2025-01-14T17:27:47Z

.github/workflows/report.yml

From

tbx workflow install report

without modification

tomuben · 2025-01-14T17:28:27Z

.github/workflows/gh-pages.yml

From

tbx workflow gh-pages

without modification

tomuben · 2025-01-14T17:29:58Z

.github/workflows/checks.yml

Mostly from

tbx workflow install checks

Only removed execution of unit test + upload of coverage -> Is now handled in tests_with_converage.yml

tomuben · 2025-01-14T17:32:27Z

.github/workflows/tests_with_coverage.yml

  Slow-Tests:
    name: Slow
+    if: github.event_name != 'pull_request'


This is not ideal, but ci.yml is also executed by a CRON schedule, and it would hang otherwise. I'am open for suggestions.

What do you mean with hang?

And the condition not pull request would deactivate this job in pull requests events. cron is a different event.

What we actually want is the deactivate the approval for automated checks like cron or push to main

I think, that is probably only possible if we split up the tests into two workflow files, one for fast and one for slow and add the manual approval in the caller and for the automated stuff we have a different caller as for the PR CI. I see iteration three coming of the plan

tkilias · 2025-01-14T18:11:01Z

.github/workflows/ci.yml

+    name: Allow Merge
+    runs-on: ubuntu-latest
+    # If you need additional jobs to be part of the merge gate, add them below
+    needs: [ Tests ]


Suggested change

needs: [ Tests ]

needs: [ checks, tests ]

Alternative you can also make Approval depend on metrics and metrics depends on tests and checks

Did the second option.

.github/workflows/ci.yml

tkilias · 2025-01-14T18:18:12Z

.github/workflows/ci.yml


  Metrics:
-    needs: [ CI ]
+    needs: [ Tests ]


Suggested change

needs: [ Tests ]

needs: [ checks, tests ]

Did the second option from comment above

ckunki · 2025-01-15T08:13:05Z

.github/workflows/ci.yml

+    name: Allow Merge
+    runs-on: ubuntu-latest
+    # If you need additional jobs to be part of the merge gate, add them below
+    needs: [ metrics ]


Allow merge should not depend on report / metrics.
We still have incidents in which metrics / KPIs / report fails for some reason and want to be able to develop and merge PRs, though.
So for now: creating the reports is optional and should not prevent to merge.

Suggested change

needs: [ metrics ]

needs: [ checks, tests ]

To make dependencies clear, I propose to move job metrics to the end of the file.

ckunki · 2025-01-15T08:27:28Z

.github/workflows/ci.yml

+    uses: ./.github/workflows/tests_with_coverage.yml
+
+  metrics:
+    needs: [ tests, checks ]


Suggested change

needs: [ tests, checks ]

needs: [ checks, tests ]

ckunki · 2025-01-15T08:30:28Z

.github/workflows/pr-merge.yml

@@ -23,3 +23,6 @@ jobs:
  metrics:
    needs: [ ci-job ]
    uses: ./.github/workflows/report.yml
+


As tests have been moved out from checks.yml, then secrets no longer need to be inherited to checks.

On the other hand: As tests have been moved out from checks.yml, then pr-merge maybe needs to execute tests individually?

Also the name of ci-job should maybe changed to "checks", then?

ckunki · 2025-01-15T08:31:42Z

.github/workflows/pr-merge.yml

@@ -23,3 +23,6 @@ jobs:
  metrics:
    needs: [ ci-job ]
    uses: ./.github/workflows/report.yml
+
+  publish-docker-runnmer:


Suggested change

publish-docker-runnmer:

publish-docker-runner:

ckunki · 2025-01-15T08:34:19Z

.github/workflows/tests_with_coverage.yml

@@ -7,39 +7,48 @@ on:
        required: false

 jobs:
+  build-matrix:
+    name: Generate Build Matrix
+    uses: ./.github/workflows/matrix-python.yml

  Tests:


We should use unified names either capitalized or lower case but not mixed.

This job only seems to execute unit tests and should be named accordingly.

ckunki · 2025-01-15T08:39:14Z

.github/workflows/tests_with_coverage.yml

  Slow-Tests:
    name: Slow
+    if: github.event_name == 'pull_request'


I propose to rename the check for manual approval, as it does not actually execute tests but only asks for a manual approval and other jobs then depend on this approval with needs.

needs: Slow-Tests sounds strange.

Candidates:

needs: Approve-Slow-Tests

needs: Manual-Approval

ckunki · 2025-01-15T08:41:10Z

.github/workflows/tests_with_coverage.yml

+    uses: ./.github/workflows/test_db_versions_all_tests.yml
+  Test-Python-Versions:
+    needs: Slow-Tests
+    uses: ./.github/workflows/test_python_version.yml


I find the name test_python_version.yml of the script not very expressive.

The job inside says

name: Run Tests with Python ${{ matrix.python_version }} and Exasol ${{ matrix.exasol_version }}

Still, I cannot tell, if these tests overlap with other groups of tests (e.g. include unit tests) or in which way they are different. Additionally, matrix-python.yml creates build matrix including multiple python versions. In which way are these different from test_python_version.yml?

1. ci.yml and periodic_validation.yml 2. Slow tests and fast tests

tkilias · 2025-01-15T17:08:53Z

.github/workflows/ci.yml

+    uses: ./.github/workflows/fast_tests.yml
+
+  metrics:
+    needs: [ fast-tests, checks ]


Usually we would have slow-tests here too

I think we should use the same or at least a similar name for the job as for the file.
currently metrics calls report.yml.

ckunki · 2025-01-16T08:28:24Z

.github/workflows/ci.yml

@@ -1,24 +1,56 @@
 name: CI


The workflow .github/workflows/build-and-publish.yml references different secrets:
secrets.PYPI_TOKEN and secrets.GITHUB_TOKEN.

The first one is passed as a parameter to the workflow while the second one seems to be inherited.
I propose to unify this and to only use 1 mechanism.

ckunki · 2025-01-16T08:56:56Z

.github/workflows/checks.yml

@@ -19,7 +19,7 @@ jobs:
          fetch-depth: 0

      - name: Setup Python & Poetry Environment
-        uses: exasol/python-toolbox/.github/actions/python-environment@0.17.0


Some workflows use dashes -, e.g. build-and-publish.yml some underscores _, e.g. test_db_versions_all_tests.yml.

Could you please unify all workflows to use the same separator?

I propose dashes -.

ckunki · 2025-01-16T09:41:21Z

.github/workflows/checks.yml

  Lint:
    name: Linting (Python-${{ matrix.python-version }})
-    needs: [ Version-Check ]
+    needs: [ Version-Check, build-matrix ]


Can we use unified style for job names?
Either capitalized or lower case only?
I propose capitalized with Dashes.

ckunki · 2025-01-16T10:06:01Z

.github/workflows/ci.yml

+    runs-on: ubuntu-latest
+    # If you need additional jobs to be part of the merge gate, add them below
+    needs: [ metrics, slow-tests ]
+


Allow Merge should not depend on metrics

…h_merge_gate_from_tbx_for_slow_tests_1 # Conflicts: # doc/changes/unreleased.md

…rmatting

ckunki · 2025-01-16T11:54:09Z

.github/workflows/ci.yml

+    uses: ./.github/workflows/fast_tests.yml
+
+  metrics:
+    needs: [ fast-tests, checks ]


ckunki · 2025-01-16T11:56:29Z

.github/workflows/ci.yml

+    uses: ./.github/workflows/fast_tests.yml
+
+  metrics:
+    needs: [ fast-tests, checks ]


I think we should use the same or at least a similar name for the job as for the file.
currently metrics calls report.yml.

tomuben marked this pull request as draft November 25, 2024 10:57

tomuben added 2 commits January 14, 2025 12:17

#427: Use GH merge-gate from TBX for slow tests

0f18b6b

Simplifed CI

a55c379

tomuben force-pushed the refactoring/427_use_gh_merge_gate_from_tbx_for_slow_tests branch from 7400ccb to a55c379 Compare January 14, 2025 15:18

tomuben temporarily deployed to manual-approval January 14, 2025 15:19 — with GitHub Actions Inactive

tomuben had a problem deploying to manual-approval January 14, 2025 15:21 — with GitHub Actions Failure

Removed unecessary approve job in CI.yml

2d3f225

tomuben had a problem deploying to manual-approval January 14, 2025 15:49 — with GitHub Actions Failure

tomuben marked this pull request as ready for review January 14, 2025 15:50

tomuben changed the title ~~#427: Use GH merge-gate from TBX for slow tests~~ #427: Use GH approved for slow tests Jan 14, 2025

Added changelog and fixed pull_request_template.mdr

b63be38

tomuben had a problem deploying to manual-approval January 14, 2025 15:55 — with GitHub Actions Failure

tomuben added 2 commits January 14, 2025 13:52

Applied GH workflows from latest toolbox (0.20.0)

3f6eb6a

Removed tests + coverage from checks.yml

ff8b57e

and use matrix-python.yml in tests_with_coverage.yml

tomuben had a problem deploying to manual-approval January 14, 2025 17:07 — with GitHub Actions Failure

Fixed formatting

8b22d34

tomuben had a problem deploying to manual-approval January 14, 2025 17:20 — with GitHub Actions Failure

Run Slow-Tests only if PR

c2f5d3b

tomuben temporarily deployed to manual-approval January 14, 2025 17:24 — with GitHub Actions Inactive

tomuben commented Jan 14, 2025

View reviewed changes

.github/workflows/report.yml Outdated

Copy link

Collaborator Author

tomuben Jan 14, 2025

Choose a reason for hiding this comment

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

From

tbx workflow install report

without modification

tomuben commented Jan 14, 2025

View reviewed changes

.github/workflows/gh-pages.yml Outdated

Copy link

Collaborator Author

tomuben Jan 14, 2025

Choose a reason for hiding this comment

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

From

tbx workflow gh-pages

without modification

tomuben commented Jan 14, 2025

View reviewed changes

tomuben changed the title ~~#427: Use GH approved for slow tests~~ #427: Implemented GH approval for slow tests Jan 14, 2025

tkilias reviewed Jan 14, 2025

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

tkilias reviewed Jan 14, 2025

View reviewed changes

Fixes from review

abd3bda

ckunki requested changes Jan 15, 2025

View reviewed changes

Split up:

776aff1

1. ci.yml and periodic_validation.yml 2. Slow tests and fast tests

tomuben had a problem deploying to manual-approval January 15, 2025 16:20 — with GitHub Actions Failure

Fix Github job name

daddfb6

tomuben temporarily deployed to manual-approval January 15, 2025 16:34 — with GitHub Actions Inactive

tkilias reviewed Jan 15, 2025

View reviewed changes

ckunki requested changes Jan 16, 2025

View reviewed changes

tomuben added 5 commits January 16, 2025 08:47

Merge remote-tracking branch 'origin/main' into refactoring/427_use_g…

53fd5db

…h_merge_gate_from_tbx_for_slow_tests_1 # Conflicts: # doc/changes/unreleased.md

Updared lock file

c0ffcd3

Fixes from Review (together with KK)

b78bb8e

Fixed workflow paths

7ea138f

Fixed workflow paths

b0f74aa

tomuben had a problem deploying to manual-approval January 17, 2025 11:21 — with GitHub Actions Failure

Renamed Job name "Slow Approval" -> "Approve Slow Tests" and fixed fo…

a6e935a

…rmatting

tomuben temporarily deployed to manual-approval January 17, 2025 11:25 — with GitHub Actions Inactive

ckunki approved these changes Jan 17, 2025

View reviewed changes

tomuben merged commit c3c7fef into main Jan 17, 2025
117 checks passed

tomuben deleted the refactoring/427_use_gh_merge_gate_from_tbx_for_slow_tests branch January 17, 2025 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#427: Implemented GH approval for slow tests #428

#427: Implemented GH approval for slow tests #428

tomuben commented Nov 22, 2024 •

edited

Loading

tomuben Jan 14, 2025 •

edited

Loading

tomuben Jan 14, 2025

tomuben Jan 14, 2025

tomuben Jan 14, 2025

tomuben Jan 14, 2025

tkilias Jan 14, 2025

tkilias Jan 14, 2025

tkilias Jan 14, 2025

tkilias Jan 14, 2025

tkilias Jan 14, 2025 •

edited

Loading

tkilias Jan 14, 2025

tomuben Jan 14, 2025

tkilias Jan 14, 2025

tomuben Jan 14, 2025

ckunki Jan 15, 2025

ckunki Jan 15, 2025

ckunki Jan 15, 2025

ckunki Jan 15, 2025

ckunki Jan 15, 2025

ckunki Jan 15, 2025

ckunki Jan 15, 2025

ckunki Jan 15, 2025

ckunki Jan 15, 2025

ckunki Jan 15, 2025

tkilias Jan 15, 2025

ckunki Jan 16, 2025

ckunki Jan 16, 2025

ckunki Jan 16, 2025

ckunki Jan 16, 2025

ckunki Jan 16, 2025

ckunki Jan 16, 2025

ckunki Jan 16, 2025

ckunki Jan 16, 2025

#427: Implemented GH approval for slow tests #428

#427: Implemented GH approval for slow tests #428

Conversation

tomuben commented Nov 22, 2024 • edited Loading

All Submissions:

tomuben Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkilias Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomuben commented Nov 22, 2024 •

edited

Loading

tomuben Jan 14, 2025 •

edited

Loading

tkilias Jan 14, 2025 •

edited

Loading