Use docker compose yaml for e2e tests against oss, add easy local run script - #171
Open
chrishagglund-ship-it wants to merge 3 commits into
Open
Use docker compose yaml for e2e tests against oss, add easy local run script#171chrishagglund-ship-it wants to merge 3 commits into
chrishagglund-ship-it wants to merge 3 commits into
Conversation
chrishagglund-ship-it
marked this pull request as ready for review
August 26, 2026 15:09
…mpose isolation and server-log capture on failures. - Make E2E task registration/completion safe for concurrent PR and test runs. - Fix OSS Conductor version selection in CI with a fallback for fork PRs that cannot access org variables. - Remove outdated gating from TaskUpdateV2Tests and clean up related documentation/comments.
chrishagglund-ship-it
force-pushed
the
e2e-against-conductor-with-local-script
branch
from
September 10, 2026 18:27
805e141 to
a5a3528
Compare
Ports javascript-sdk#176's resolution of the same review feedback, so the OSS harness stays diffable across the SDKs. One tag, one home. The tag was written twice: FORK_PR_FALLBACK_VERSION in the integration_tests_oss job, and `latest` as the local script's default. A local run therefore could not reproduce a CI failure, which is most of why the script exists. Rather than teach one of them to read the other, make the `image:` line of docker-compose-oss.yaml the only place it is written and let both fall through to it: the script applies no default of its own and exports OSS_CONDUCTOR_VERSION only when --version actually supplied one, and the fork-PR branch of the resolve step now leaves the variable unset instead of pinning its own copy. Fork CI and a plain local run reach the identical image by the identical path, with no YAML parsing on either side and one hardcode removed rather than a mechanism added. The non-fork empty case still fails loudly. The script's "Using ..." and "Pulling ..." lines now come from `compose config --images conductor-server` instead of reconstructing the tag, so they stay honest whichever source supplied it. Worth stating plainly: E2E_TEST_OSS_CONDUCTOR_VERSION is currently set to `latest` at the org level, so the pin is nominal for normal CI runs until someone sets it to a real version. The compose default is what actually holds the line today, on fork PRs and locally. Pull in CI. The script pulls before `up`, CI did not. On a GitHub-hosted runner the VM is ephemeral and starts with no cached copy, so `up` pulls anyway and the step is redundant today — kept regardless, because it costs no extra network pull (`up` then finds the image locally), it splits "couldn't pull the image" from "the stack didn't come up" into two distinct red steps, and it is what stops a mutable tag going stale the day this job moves to a self-hosted runner with a warm Docker daemon. It also prints the tag in use, which matters now that a fork PR's tag is not spelled out in the workflow. It sits after Checkout because it needs the compose file, and before the stack starts; .NET setup deliberately stays after the stack is up, as before. Not touched: agent-e2e.yml's CONDUCTOR_SERVER_VERSION. That is a Maven Central artifact version for the conductor-server boot JAR, not a Docker Hub image tag, and it is out of scope here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The "Using ..." line printed `postgres:16`. `docker compose config --images` lists every service's image and does not reliably honour the service-name filter it accepts, so `--images conductor-server | head -1` took whichever image compose happened to emit first -- the postgres one. Observed in java-sdk against the same compose layout. Cosmetic only: `compose pull conductor-server` and `compose up` both address the service directly and were always correct, so the stack has been running the right image throughout; only the echo was wrong. Select by image name instead. That couples to `conductoross/conductor`, which the line being replaced already hardcoded, so nothing new is pinned down. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces the inline heredoc that the
integration_tests_ossjob used to writedocker-compose-oss.yamlon the fly with a committed compose file, and adds a script so the same stack and test filter can be run locally.scripts/docker-compose-oss.yaml(new) — Conductor OSS + Postgres 16, image tag from$OSS_CONDUCTOR_VERSION. Itsimage:line is the single place the default tag is written; CI overrides it fromvars.E2E_TEST_OSS_CONDUCTOR_VERSION(still guarded by the existing hard-fail step), while fork PRs and local runs both fall through to that default.scripts/run-integration-oss.sh(new) — pulls the image (so a cached mutable tag can't silently go stale), starts the stack, waits for/health, runsCategory=Integration&ServerType!=Orkes, tears down on exit. Supports--version <tag>and--keep-up..github/workflows/pull_request.yml— drops the heredoc step and points theup/logssteps at the committed file, adds apullstep beforeup, and removes the now-redundantFORK_PR_FALLBACK_VERSION(fork PRs take the compose default instead). No change to the test filter ordotnet testinvocation. The steps stay separate rather than delegating to the script, so a CI failure still shows which stage broke.docs/workflow-testing.md— how to run the OSS suite locally, and why the Orkes-only tests are filtered out.Tests/Integration/{Environment/EnvironmentVariableTests,Workflow/WorkflowLifecycleTests}.cs— comments only. Records, next to each[Trait("ServerType", "Orkes")], which OSS endpoint is missing and why the test is gated, so the reason is visible to whoever edits the gate:EnvironmentVariableTests— OSS'sEnvironmentResourceis read-only (GET /api/environment,GET /api/environment/{key}, added by Secrets & environment: workflow references (${workflow.secrets/env}) + task-declared runtimeMetadata conductor#1251, first released in 3.32.0-rc.5 and 3.31.2), soPUT /environment/{key}returns 405.UpdateWorkflowVariables_VariablesAreReflected—PUT /workflow/{workflowId}/variablesis not a registered OSS route, so it returns 404. Unrelated to theSET_VARIABLEtask type, which OSS does support.Follow-up (test isolation, not SDK behavior): After running the suite against OSS, it was observed that tests that poll by task type could claim (and complete) a task from another execution — a leftover from a failed attempt, a re-run, or a concurrent PR. TestPrefix now includes GITHUB_RUN_ATTEMPT, poll/complete/cleanup only act on the workflow that started the task, and failed tests still tear down. Also dropped the unused [Trait("Version", "V5Only")] on TaskUpdateV2Tests: that filter isn’t applied by the OSS job, and the class never calls task-update-v2.