quality: parameterize branch names and extract test action by typotter · Pull Request #65 · Eppo-exp/python-sdk

typotter · 2024-08-21T15:55:09Z

🎟️ Fixes FF-3094 towards FF-3085

👯‍♂️ Related PRs

Motivation

Changes are often made to the sdk-test-data repository to capture new behaviours, bugs and edge cases. When these changes are pushed to main, the SDKs are cloned locally (locally to the github action running) and their respective tests are run. These tests are set up and run by copies of the SDK test workflows - see sdk-test-data workflow. There are a number of limitations to this setup:

Test steps are copied from the SDK’s respective workflows; changes to the SDK test workflows need to be replicated in sdk-test-data and this is not obvious to devs
The SDK tests are not able to run against in-flight changes to sdk-test-data
When new test-data is committed that breaks an SDK, that breakage is not surfaced in the SDK’s repository until some action triggers its testing workflow (on demand, on pull-request, etc.).

Description of Changes

This change
🚀 - The test job is extracted into a composite action, allowing it to be called from both the SDK's local workflow as well as from workflows in other repositories.

External to this Change
sdk-test-data get two testing workflows.

♻️ "Local Testing"- For all pull request changes, the "Local Testing" workflow calls the reusable SDK workflows, test results are recorded only in the sdk-test-data action. This is run using the main SDK branch and the "current" branch of workflow, i.e. the pull request branch. (SDK repo does not see/is not notified of test failures during PR lifecycle, only on push to main)
♻️ 🧑‍💻 "Remote Testing" - On all pushes to the main branch of sdk-test-data, the SDK testing workflows are triggered to run within their respective repositories, alerting all subscribers of failed runs (repo is red/green).

typotter · 2024-08-21T16:35:08Z

.github/workflows/lint-test-sdk.yml

-        uses: 'google-github-actions/setup-gcloud@v0'
-      - name: "Run tests"
-        run: make test
+      - uses: Eppo-exp/python-sdk/.github/actions/action-test@tp/workflows/remote


temp branch reference.

Would a relative path work here and be more concise?

yes, yes it would, however, it requires an additional actions/checkout step to checkout the repo first, which is a step that needs to be done in the action, so it adds redundant work.

typotter · 2024-08-21T16:38:30Z

.github/workflows/lint-test-sdk.yml

  pull_request:
    paths:
      - '**/*'
+  workflow_dispatch:


allows workflow to be triggered remotely via API

I like the rename to be consistent with other SDKs as well 🧹

typotter · 2024-08-21T16:39:08Z

.github/workflows/lint-test-sdk.yml

  run-tests-with-tox:
    runs-on: ubuntu-latest
    steps:
-      - uses: "actions/checkout@v2"


extracted into a composite action. Keeps the main workflow file cleaner in exchange for some more boilerplate and yet another yaml file

aarsilv · 2024-08-23T19:47:47Z

.github/workflows/lint-test-sdk.yml

  pull_request:
    paths:
      - '**/*'
+  workflow_dispatch:


I like the rename to be consistent with other SDKs as well 🧹

aarsilv · 2024-08-23T19:49:01Z

.github/workflows/lint-test-sdk.yml

-        uses: 'google-github-actions/setup-gcloud@v0'
-      - name: "Run tests"
-        run: make test
+      - uses: Eppo-exp/python-sdk/.github/actions/action-test@tp/workflows/remote


Would a relative path work here and be more concise?

aarsilv · 2024-08-23T19:49:25Z

.github/actions/action-test/action.yml

@@ -0,0 +1,46 @@
+name: 'Test SDK'


Should we rename this file? action.yml feels kind of generic

I might have read the docs wrong, but as I understand it, github expects action.yml for your action magic. The naming is in its containing directory where common convention is to prefix with action-.

oh right this is an action not a workflow, carry on

aarsilv

👍

rasendubi · 2024-08-30T14:06:59Z

.github/workflows/lint-test-sdk.yml

-        uses: 'google-github-actions/setup-gcloud@v0'
-      - name: "Run tests"
-        run: make test
+      - uses: Eppo-exp/python-sdk/.github/actions/action-test@tp/workflows/remote


@typotter you seem to forgot removing temp branch reference before merging? ^

This broke PR testing: https://github.com/Eppo-exp/python-sdk/actions/runs/10634064395/job/29480558050?pr=68

typotter · 2024-08-30T14:21:11Z

Thanks Oleksii. On it.

…

On Fri, Aug 30, 2024, 8:07 a.m. Oleksii Shmalko ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In .github/workflows/lint-test-sdk.yml <#65 (comment)>: > @@ -50,17 +51,6 @@ jobs: run-tests-with-tox: runs-on: ubuntu-latest steps: - - uses: ***@***.***" - - uses: ***@***.***" - with: - python-version: '3.9.x' - - name: "Install dependencies" - run: | - python -VV - python -m pip install --upgrade pip setuptools wheel - python -m pip install -r requirements.txt - python -m pip install -r requirements-test.txt - - name: 'Set up GCP SDK to download test data' - uses: ***@***.***' - - name: "Run tests" - run: make test + - uses: ***@***.***/workflows/remote @typotter <https://github.com/typotter> you seem to forgot removing temp branch reference before merging? ^ This broke PR testing: https://github.com/Eppo-exp/python-sdk/actions/runs/10634064395/job/29480558050?pr=68 — Reply to this email directly, view it on GitHub <#65 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACJ5TDNQ7QQ5NQLBVXX53J3ZUB4BTAVCNFSM6AAAAABM4KRGRKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDENZSGQ3DQOJTHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

typotter mentioned this pull request Aug 21, 2024

quality: Round 2 of SDK test workflow migrations Eppo-exp/sdk-test-data#58

Merged

typotter changed the title ~~Tp/workflows/remote~~ quality: parameterize branch names and extract test action Aug 21, 2024

typotter force-pushed the tp/workflows/remote branch from fa7349e to 6263c23 Compare August 21, 2024 16:08

Extract action

60bea96

typotter force-pushed the tp/workflows/remote branch from 6263c23 to 60bea96 Compare August 21, 2024 16:16

typotter added 3 commits August 21, 2024 10:23

allow main test workflow to be callable

53826c9

consistent workflow name

317e719

API uses dispatch, not call

4775dbb

typotter commented Aug 21, 2024

View reviewed changes

This was referenced Aug 21, 2024

quality: parameterize branch names and make test Eppo-exp/js-client-sdk#96

Merged

quality: Parameterize branch names for test workflow Eppo-exp/java-server-sdk#75

Merged

typotter marked this pull request as ready for review August 21, 2024 16:38

typotter commented Aug 21, 2024

View reviewed changes

typotter requested review from aarsilv, leoromanovsky and petzel August 21, 2024 16:39

leoromanovsky approved these changes Aug 22, 2024

View reviewed changes

aarsilv approved these changes Aug 23, 2024

View reviewed changes

aarsilv assigned typotter Aug 23, 2024

typotter added 4 commits August 29, 2024 08:55

relative path

17c825c

abs path

e907e3c

trim params

e10ae06

untrim params

f882436

typotter requested a review from aarsilv August 29, 2024 15:06

typotter assigned aarsilv and unassigned typotter Aug 29, 2024

aarsilv approved these changes Aug 29, 2024

View reviewed changes

typotter merged commit c534159 into main Aug 30, 2024

typotter deleted the tp/workflows/remote branch August 30, 2024 07:13

rasendubi reviewed Aug 30, 2024

View reviewed changes

Conversation

typotter commented Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Description of Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarsilv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

typotter commented Aug 30, 2024 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

typotter commented Aug 21, 2024 •

edited

Loading