Skip to content

Canary build to run nightly against JNats - #23

Merged
scottf merged 2 commits into
mainfrom
canary
Jul 28, 2026
Merged

Canary build to run nightly against JNats#23
scottf merged 2 commits into
mainfrom
canary

Conversation

@scottf

@scottf scottf commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Addresses #22

Adds a scheduled GitHub Actions workflow (jnats-canary.yml) that builds and runs the full test suite against jnats' current main snapshot once a day, across all supported Java target levels (8, 17, 21, 25).

Why

java-active-passive is tightly coupled to jnats, so a change upstream can break us before we've bumped our pinned version. This canary catches that early — we find out the morning after a breaking jnats change, in our own CI, rather than when we go to adopt a release.

How

It reads jnats' current version from jnats main, tests against -SNAPSHOT (rewriting the dependency for the run only — nothing committed), and reports green/red. No credentials and no changes on the jnats side — it just reads jnats' public published artifacts. A status badge in the README shows the current state at a glance, and GitHub emails on failure.

Etc.

Can be run on demand from the Actions tab, optionally against a specific version (e.g. to vet a jnats release before adopting it).

@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Jul 28, 2026

Copy link
Copy Markdown

Claude finished @scottf's task in 2m 14s —— View job


PR Review

  • Gather context
  • Analyze PR diff
  • Review workflow YAML
  • Post inline comments and summary

The workflow design is solid: minimal permissions (contents: read), fail-fast: false to get full matrix results, --refresh-dependencies to bypass Gradle's snapshot cache, and the version resolved once in a dedicated job so it's consistent across all matrix legs. Two issues worth addressing:

Medium — missing set -e in resolve step (line 35)
Without errexit, a curl failure leaves JARVER empty and the job outputs version=-SNAPSHOT. The test job will then attempt io.nats:jnats:-SNAPSHOT, causing a misleading Gradle resolution error rather than a clear "couldn't fetch version" failure. Fix: set -euo pipefail.

Low — shell injection via direct ${{ }} interpolation (line 36)
VERSION='${{ github.event.inputs.version }}' embeds the input directly into the script source before the shell sees it. A value containing a single quote breaks out of the string. Low risk given the private repo and restricted workflow_dispatch access, but the safe fix is to pass the input through an env var instead.

Everything else looks good — the sed replacement pattern, the matrix configuration matching build-pr.yml, and the version fetch approach are all correct.

Comment thread .github/workflows/jnats-canary.yml Outdated
- name: Resolve the jnats version to test
id: v
run: |
set -uo pipefail

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing -e flag. Without set -e, if curl fails (network outage, GitHub unavailable), the pipeline inside $(...) returns non-zero but the assignment itself succeeds with JARVER="". Execution continues silently and the resolve job outputs version=-SNAPSHOT, which Gradle will reject with a confusing dependency resolution error rather than a clear "couldn't fetch version" failure.

Suggested change
set -uo pipefail
set -euo pipefail

Comment thread .github/workflows/jnats-canary.yml Outdated
id: v
run: |
set -uo pipefail
VERSION='${{ github.event.inputs.version }}'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shell injection risk: ${{ github.event.inputs.version }} is interpolated directly into the script text before the shell runs it. A value like foo' && malicious_command ' would break out of the single-quoted string.

The safe pattern is to pass the input as an environment variable so the value is never embedded in the script source:

Suggested change
VERSION='${{ github.event.inputs.version }}'
VERSION="${INPUT_VERSION:-}"

And add to the job (or step) env block:

env:
  INPUT_VERSION: ${{ github.event.inputs.version }}

For a private repo with restricted workflow_dispatch access this is low-severity, but it's the recommended GitHub hardening practice.

@scottf

scottf commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator Author

@claude re-review

@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Jul 28, 2026

Copy link
Copy Markdown

Claude finished @scottf's task in 1m 47s —— View job


Re-review

  • Gather context
  • Read current workflow file
  • Check if previous issues were addressed
  • Fresh review pass
  • Post findings

Both issues from the previous review have been addressed cleanly.

Previous findings — resolved

# Severity Finding Status
1 Medium Missing set -e ✅ Fixed: set -euo pipefail on line 37
2 Low Shell injection via ${{ }} in script body ✅ Fixed: version passed through INPUT_VERSION env var (lines 35, 38)

The comment on line 35 (# passed as data, never interpolated into the script) is a nice touch — makes the intent explicit for future readers.


Fresh pass — new observations

Low — nats-server compiled from source on every matrix leg

Lines 74-79 run go build inside each of the 4 parallel matrix jobs. A Go compile takes 2-3 minutes, so this adds ~8-12 minutes of serial build time across the matrix (though they run in parallel, each leg still waits on its own compile before tests start). Using a pre-built binary from the nats-server GitHub releases would be faster and more reproducible. Example:

- name: Install Nats Server
  run: |
    NATS_VER=$(curl -sf https://api.github.com/repos/nats-io/nats-server/releases/latest | grep -oP '"tag_name": "\K[^"]+')
    curl -sfL "https://github.com/nats-io/nats-server/releases/download/${NATS_VER}/nats-server-${NATS_VER}-linux-amd64.tar.gz" \
      | tar -xz --strip-components=1 -C ~/.local/bin nats-server-${NATS_VER}-linux-amd64/nats-server
    nats-server -v

This is a performance note, not a correctness issue — the build-from-source approach is correct and more likely to catch integration issues with the very latest nats-server.

Informational — ~/.local/bin in PATH for subsequent steps

The binary is installed to ~/.local/bin and verified with nats-server -v in the install step. On ubuntu-latest runners, ~/.local/bin is typically in $PATH for all run steps via the runner's environment setup, so this should work. Just worth confirming once the workflow runs live that the Gradle test process can find nats-server by name (if it invokes it as a subprocess). If it can't, a echo "$HOME/.local/bin" >> $GITHUB_PATH line in the install step would fix it.


Overall

The two issues from the prior review are properly resolved. The workflow design is sound: correct permissions (contents: read), fail-fast: false, --refresh-dependencies, version resolved once and passed cleanly through outputs. The new observations above are minor polish — the workflow is ready to merge.

@scottf
scottf merged commit 959422e into main Jul 28, 2026
5 checks passed
@scottf
scottf deleted the canary branch July 28, 2026 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant