build: bump conductor-go to v1.10.2 and un-skip schedule pause/resume on OSS (fixes #101) - #107
Conversation
|
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
mp-orkes
left a comment
mp-orkes
left a comment
There was a problem hiding this comment.
The correct fix for this is in the SDK.
Review feedback from @kowser-orkes on #107. Narrows the fallback trigger from any 4xx to exactly 405. 405 is the only status that means "wrong verb"; other 4xx have distinct causes and retrying them with GET was wrong: 404 the schedule, or the whole scheduler module, is absent. The retry produced a second 404 and reported that, masking the actionable hint added in #86. 401 auth failure. The retry simply repeated the rejection. Confirmed against OSS Conductor: wrong verb returns 405, a missing schedule and a missing endpoint both return 404 — so the previous range genuinely conflated them. `conductor schedule pause does_not_exist` now surfaces the scheduler-module hint again instead of a fallback-induced error. Also trims the doc comment to the three lines that carry information a reader cannot get from the code. Tests gain 404 and 401 cases asserting no fallback is attempted, alongside the existing 405-falls-back and 5xx-does-not cases. Re-verified end to end against Conductor OSS built from main: pause sets "paused": true, resume clears it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to review feedback on the downstream CLI change (conductor-oss/conductor-cli#107), applied here so the two implementations do not diverge. 405 is the only status that means "wrong verb". The previous 4xx range also caught statuses with unrelated causes, and retrying those with GET was wrong: 404 the schedule, or the whole scheduler module, is absent — the retry produced a second 404 and reported that instead of the real cause 401 auth failure — the retry simply repeated the rejection Verified against OSS Conductor: a wrong verb returns 405, while a missing schedule and a missing endpoint both return 404, so the range genuinely conflated them. Tests gain 404 and 401 cases asserting no fallback is attempted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both addressed in 67c8300 — thanks, the 405 point was a real correctness fix, not just tidying. Narrowed to 405. You were right that the 4xx range was too broad. Checked what OSS actually returns:
So the range conflated "wrong verb" with "not found". Concretely, Added tests asserting 404 and 401 do not trigger a fallback, alongside the existing 405-does and 5xx-does-not cases. Comment trimmed to three lines. Also applied the same 405 narrowing to the SDK PR (conductor-oss/go-sdk#276) so the two implementations don't diverge — see the note below on which one we keep. On placement: @mp-orkes asked for this to live in the SDK instead, and he's right — I've opened conductor-oss/go-sdk#276 with the same logic in This PR is therefore likely to be closed in favour of a |
| // setSchedulePaused pauses or resumes a schedule. OSS Conductor accepts only PUT on | ||
| // these endpoints, older Orkes deployments only GET, so try PUT and fall back to GET | ||
| // on 405. Any other status is returned as-is. | ||
| func setSchedulePaused(ctx context.Context, name, action string) error { |
There was a problem hiding this comment.
if you are making these changes conductor-oss/go-sdk#276, let's go with that instead of setSchedulePaused.
|
@mp-orkes I'd like your call rather than guess. So the fix definitely lands in the SDK, which is in this Go SDK PR #276. The open question is whether this CLI PR should be closed or kept as a fallback. |
@ambiorix2099 tell your agent, I want a human reply. |
We need a CLI PR to update the SDK and make the scheduler tests run in OSS, not skip. |
…276) * fix(scheduler): PUT with GET fallback for per-schedule pause/resume PauseSchedule and ResumeSchedule issue a GET, but upstream OSS Conductor declares both endpoints @PutMapping, so every call against an OSS server fails with 405 "Request method 'GET' is not supported". A straight GET -> PUT swap is not safe, because the accepted verb varies by deployment: OSS Conductor PUT only (GET -> 405) Orkes Conductor >= 2026-07-14 PUT or GET Orkes Conductor < 2026-07-14 GET only (PUT -> 405) Orkes gained PUT in orkes-conductor 1854375f0c (2026-07-14); anything older still needs GET. Both methods now go through putThenGet, which issues PUT and falls back to GET only when the server refuses with a 4xx. That keeps existing callers working against every deployment while converging on the RESTful verb, so the fallback can be deleted once no supported server predates PUT. The fallback is limited to 4xx on purpose: retrying a 5xx or a transport error with a different verb would mask the real fault and report it as a method problem. Tests model all three deployment generations plus neither-verb-accepted with stub servers that record the verbs received, asserting that PUT is preferred and GET is used only when PUT is refused. Also covers the 5xx no-fallback rule and that resume targets its own endpoint. Reported downstream as conductor-oss/conductor-cli#101. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(scheduler): narrow the GET fallback to 405 Follow-up to review feedback on the downstream CLI change (conductor-oss/conductor-cli#107), applied here so the two implementations do not diverge. 405 is the only status that means "wrong verb". The previous 4xx range also caught statuses with unrelated causes, and retrying those with GET was wrong: 404 the schedule, or the whole scheduler module, is absent — the retry produced a second 404 and reported that instead of the real cause 401 auth failure — the retry simply repeated the rejection Verified against OSS Conductor: a wrong verb returns 405, while a missing schedule and a missing endpoint both return 404, so the range genuinely conflated them. Tests gain 404 and 401 cases asserting no fallback is attempted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(scheduler): correct the stale 4xx wording and trim the comments Review feedback from @mp-orkes on #276. The doc comment still said the fallback triggers on "4xx" after the code was narrowed to 405 — the earlier commit updated the later paragraphs and missed the opening sentence, so the documentation contradicted the implementation. Fixed. Trimmed the surrounding commentary: dropped the deployment table, the @PutMapping reference (irrelevant in Go — that it is a PUT is the only part that matters), and the in-function comment that restated the doc comment. Same for the test file, where the per-case notes repeated what the case names already said. Net -27 lines of comment with no change in behaviour; tests unchanged and passing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Removes the skip_if_oss_101 guard so schedule.bats tests 15 and 16 exercise pause/resume in the OSS venue rather than skipping there. The guard was correct while #101 was open — the SDK sent GET where OSS requires PUT, so both tests failed. The fix landed in the SDK (conductor-oss/go-sdk#276), so the guard is now a skip that outlives its defect: CI would report green over the two tests that cover exactly the behaviour we fixed. Requires the accompanying go.mod bump to a conductor-go version containing that fix. Verified locally with the SDK change in place: schedule.bats is 22/22 against Conductor OSS built from main (tests 15 and 16 passing rather than skipped), and the full OSS selection is 109 passed / 0 failed, with skips down from 12 to 10. Without the bump these two tests fail on OSS, so this must not merge ahead of it. Refs #101
v1.10.2 contains the PUT-with-GET-fallback for per-schedule pause/resume (conductor-oss/go-sdk#276), which is what makes those two commands work against OSS Conductor. Without it the SDK issues a GET where OSS requires PUT and both return 405. The tag points at the merge commit b7135168, and the fallback is present in the module as published — checked rather than assumed, since the jump from v1.8.0 crosses two minor versions. Verified against Conductor OSS built from main: schedule.bats is 22/22 with tests 15 and 16 passing rather than skipped, and the full OSS selection is 109 passed / 0 failed. go build, go vet and the unit suites are clean. Fixes #101
67c8300 to
e34d0f4
Compare
ambiorix2099
changed the title
5 participants
Pull Request type
Changes in this PR
Picks up the SDK fix for #101 and stops skipping the tests that cover it.
go.mod/go.sum— bumpconductor-gofrom v1.8.0 to v1.10.2, which contains thePUT-with-GET-fallback for per-schedule pause/resume (fix(scheduler): PUT with GET fallback for per-schedule pause/resume go-sdk#276). Without it the
SDK issues a
GETwhere OSS Conductor requiresPUT, and both commands return 405.test/e2e/schedule.bats— remove theskip_if_oss_101guard, so tests 15 and 16 exercisepause/resume in the OSS venue instead of skipping there.
Earlier revisions of this PR implemented the fallback in
cmd/scheduler.go. That is gone: per@mp-orkes the fix belongs in the SDK, so the CLI now only consumes it. No CLI source changes remain.
The tag was checked rather than assumed —
v1.10.2points at merge commitb7135168, andputThenGetis present in the published module. The jump crosses two minor versions, so build, vetand the unit suites were re-run.
Verified against Conductor OSS built from
main:schedule.batsis 22/22 with 15 and 16 passingrather than skipped, and the full OSS selection is 109 passed / 0 failed. CI is green here too,
including the
E2E (local OSS server)job that runs those two tests.Issue #
Fixes #101.
Alternatives considered
Implement the fallback in
cmd/scheduler.go, as earlier revisions did. Rejected on @mp-orkes'review: it belongs in the SDK, where every consumer benefits rather than this CLI alone. Keeping
both would have meant the same logic in two layers, with the CLI's copy shadowing the SDK's.
Pin a pseudo-version of the merge commit instead of a tag. Unnecessary — v1.10.2 was cut from that
commit, so a released version is available.
Leave the
skip_if_oss_101guard in place. Rejected: the two tests covering this behaviour wouldstay skipped on OSS and CI would report green over the thing we just fixed.