-
Notifications
You must be signed in to change notification settings - Fork 2
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
quality: Round 2 of SDK test workflow migrations #58
Conversation
.github/workflows/test-sdks.yml
Outdated
@@ -1,4 +1,5 @@ | |||
name: Test SDKs Locally | |||
name: Test SDKs Locally |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this line was duplicated by accident
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the catch; late night merges...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving as-is, but I'd consider using a matrix to simplify/consolidate things
@@ -4,9 +4,30 @@ on: | |||
push: | |||
branches: | |||
- main | |||
- tp/workflows/round2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
temporary I'm guessing?
What about running these shared tests on all pull requests, like we do for the individual SDKs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test-sdks.yml
is for in-flight changes; we don't want to make the SDKs in their respective repos until the test cases are checked in.
Temporary indeed; removed.
workflow_dispatch: | ||
|
||
jobs: | ||
|
||
test-java-sdk: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lots of duplicated code, have you considered using a matrix?
Unchecked example from ChatGPT:
test-sdks:
runs-on: ubuntu-latest
strategy:
matrix:
sdk:
- { repo: "java-server-sdk", workflow: "lint-test-sdk.yml", ref: "tp/workflows/remote" }
- { repo: "php-sdk", workflow: "run-tests.yml", ref: "main" }
- { repo: "js-client-sdk", workflow: "lint-test-sdk.yml", ref: "tp/workflows/remote" }
- { repo: "python-sdk", workflow: "lint-test-sdk.yml", ref: "tp/workflows/remote" }
steps:
- name: Display workflow details
shell: bash
run: |
echo "Testing eppo-exp/${{ matrix.sdk.repo }}"
- name: Run remote workflow
uses: convictional/[email protected]
with:
owner: Eppo-exp
repo: ${{ matrix.sdk.repo }}
workflow_file_name: ${{ matrix.sdk.workflow }}
ref: ${{ matrix.sdk.ref }}
github_token: ${{ secrets.AUTH_TOKEN }}
wait_interval: 10
propagate_failure: true
trigger_workflow: true
wait_workflow: true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't, in fact. π§βπ³
owner: Eppo-exp | ||
repo: java-server-sdk | ||
workflow_file_name: lint-test-sdk.yml | ||
ref: tp/workflows/remote |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you merge these first I imagine you won't need this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's right; will remove these refs after SDK PRs are merged
.github/workflows/test-sdks.yml
Outdated
uses: 'google-github-actions/setup-gcloud@v0' | ||
- name: Run tests | ||
run: make test | ||
uses: Eppo-exp/java-server-sdk/.github/workflows/lint-test-sdk.yml@tp/workflows/remote |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the eventual plan to remove this file and just use test-sdks-remote
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the remote workflow is to trigger the tests to run in the respective SDK repos, effectively turning them red when the test data causes a test failure. The test-sdks.yml
runs the SDK tests but in the sdk-test-data repo, so it doesn't cause spurious notifications from the repos.
ποΈ Fixes FF-3088 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:sdk-test-data
Description of Changes
This change
π - Each SDK's testing workflow is enhanced into a reusable workflow, exposing parameters for SDK branch and the sdk-test-data branch to use in testing.
External to this Change
sdk-test-data get two testing workflows.