Bugfix action deployment matching - #30
Merged
Merged
Conversation
The "Mark GitHub deployment inactive" step listed deployments by environment and marked all of them inactive. environment defaults to name (unique per preview) but is user-overridable, and the README/ inputs docs advertise that override — so several previews sharing an explicit environment: value would have one down call blow away every other preview's deployment status too. sha isn't a usable join key either: down runs on pull_request:closed with whatever sha that event carries, which has no guaranteed relationship to the sha(s) recorded by preceding up calls (synchronize reruns up with a new sha each time). name is the one value the action already requires to stay identical between up and down, so tag deployments with task="cfzt:<name>" on creation and filter on it (client-side, after the environment-scoped list call) before marking inactive.
zt up creating a live tunnel while the Deployments API call fails left the tunnel orphaned — nothing in the workflow would ever tear it down, since the paired "down" run resolves against a deployment (task=cfzt:<name>) that was never created. Add a cleanup step, gated on zt-up having succeeded and record-deployment having failed (via step ids + outcome checks, with always() to survive the default skip-remaining-steps-on-failure behavior), that runs "zt down --remote" and emits a workflow warning. The job still ends up failed overall, since the record-deployment step itself failed — this only prevents the live tunnel from being left behind.
Always resolving "latest" meant the exact same install.sh invocation could pull a different zt build on two different days — no way to pin. ZT_VERSION, when set, skips the GitHub API lookup and installs that tag directly; unset behavior (resolve latest) is unchanged. Used by the next commit to let cfzt-action pin its cfzt install.
Both installs previously always tracked "latest"/main with no way to pin: cloudflared via releases/latest/download/..., cfzt via main's install.sh. Same repo commit + same action version could therefore run against a different cloudflared or zt build on two different days. Add cloudflared-version and cfzt-version inputs (both default '', preserving current latest-tracking behavior so this isn't breaking). When set: cloudflared downloads that release tag's asset directly; cfzt-version also pins which install.sh is fetched (not just what it installs via ZT_VERSION), since main's install.sh can itself change independently of any released version. A workflow warning is emitted when either is left unset, nudging toward pinning for CI reproducibility without forcing it.
The composite action assumes gh and jq are on PATH (config write, up/ down arg building, and both Deployments API steps) but never checked for either — on a self-hosted runner without them preinstalled, the first real failure showed up several steps in as a bare 'command not found', with no indication of which tool or why. Check both in Validate inputs and fail with an actionable message; jq is required unconditionally, gh only when create-deployment is true.
The Cloudflare API token written to ~/.zt-config.json for zt to read was never cleaned up after up or down finished. Harmless on GitHub-hosted ephemeral runners (the VM dies anyway) but a real leftover-secret concern on self-hosted/persistent runners. Track whether this run created the file (write-config step, created output) versus it already existing — a self-hosted runner may have its own persistent config the action shouldn't touch — and only rm -f it in an always()-gated final step when this run was the one that wrote it.
pickPort iterated a Go map (Docker's NetworkSettings.Ports) and returned whichever key came out first — map iteration order is randomized, so a container publishing more than one port (-p 8080:80 -p 8443:443) could get a different port picked by `zt up --docker` on different runs of the exact same command. Sort the tcp-suffixed keys and take the lowest container port, which matches the port a reader would expect to be "the app's port" without requiring a new flag. findByList (the /containers/json list fallback) is untouched — its Ports come back as a JSON array from dockerd itself, not a Go map, so it was never subject to this. Added TestFindContainerPort_MultiplePorts_Deterministic, which asserts the same port across 20 calls against a three-port fixture.
schtasks /end only requests termination — it returns as soon as Task Scheduler accepts the request, not once the process has actually exited. Uninstall/UninstallWatchdog immediately followed /end with /delete, so a cloudflared (or watchdog) process that was slow to die (e.g. stuck in a network wait) could still be alive and holding its port after teardown reported success, with no Task Scheduler registration left to find it by. Add waitForTaskStopped, the /end-side counterpart to the existing waitForTaskRunning, using the same taskStartupBackoff polling schedule. Both Uninstall paths now poll after /end and print a warning (not an error — teardown still proceeds with /delete, since blocking it entirely on a possibly-stuck process would be worse) if the task is still Running once the backoff is exhausted.
--remote was already gated behind an explicit flag with the trade-off explained in a code comment (down.go) and README, but SECURITY.md — the doc someone actually checks before deciding whether a flag is safe to hand to CI — said nothing about it. Spell out that it deletes by name with no ownership check, so a manually-created tunnel sharing a name with a zt-managed one is at risk if --remote is ever run outside a context that owns the name exclusively.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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
2 participants
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.
No description provided.