diff --git a/.github/RELEASE_GUIDE.md b/.github/RELEASE_GUIDE.md index 7fad49d..0b557fd 100644 --- a/.github/RELEASE_GUIDE.md +++ b/.github/RELEASE_GUIDE.md @@ -88,5 +88,5 @@ Only `feat`, `fix`, `perf`, and `refactor` commits trigger releases. Use `chore` The release workflow warms the proxy automatically. If it still shows stale data: ```bash -GOPROXY=https://proxy.golang.org go list -m github.com/jasoet/pkg/v2@v2.x.x +GOPROXY=https://proxy.golang.org go list -m github.com/jasoet/pkg/v3@v3.x.x ``` diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9230720..2083939 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2,10 +2,15 @@ name: CI on: push: - branches: [main] + branches: [main, next, 'release/v2'] tags: ['*'] pull_request: - branches: [main] + branches: [main, next, 'release/v2'] + +env: + # Pinned so a blocking API gate is reproducible. Matches the golang.org/x/exp + # pseudo-version already in go.mod; bump both together. + GORELEASE_VERSION: v0.0.0-20251113190631-e25ba8c21ef6 jobs: ci: @@ -17,9 +22,35 @@ jobs: steps: - name: Checkout uses: actions/checkout@v6 + with: + fetch-depth: 0 - name: Lint run: nix develop --command golangci-lint run ./... - name: Test - run: nix develop --command go test -race -count=1 ./... -tags=!examples + run: nix develop --command go test -race -count=1 ./... + + - name: Vet (including build-tagged code) + # Compiles and vets example/integration/argo-tagged files that the untagged + # build never sees, closing the gap where tagged code regressions slip past CI. + run: nix develop --command go vet -tags='example integration argo' ./... + + - name: API compatibility check + # Breaking API changes are intended on the v3 line, so this gate is + # informational wherever `next` is involved and blocking everywhere else: + # - ref_name == 'next' : pushes to next + # - base_ref == 'next' : PRs targeting next + # - head_ref == 'next' : the next -> main v3 release PR. Without this + # clause base_ref is 'main' and the blocking gate fires on the whole + # intended v3 break set, making the release PR unmergeable. + if: github.ref_name != 'next' && github.base_ref != 'next' && github.head_ref != 'next' + run: nix develop --command go run golang.org/x/exp/cmd/gorelease@${{ env.GORELEASE_VERSION }} + + - name: API compatibility report (informational on next) + if: github.ref_name == 'next' || github.base_ref == 'next' || github.head_ref == 'next' + # gorelease reports "no baseline version" until the first non-prerelease + # /v3 tag exists on main; that is expected for the v3 release PR and is + # why this run must not gate the merge. + continue-on-error: true + run: nix develop --command go run golang.org/x/exp/cmd/gorelease@${{ env.GORELEASE_VERSION }} diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 6984e73..fd3ae40 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -2,13 +2,14 @@ name: Release on: push: - branches: [main] + branches: [main, next, 'release/v2'] workflow_dispatch: jobs: test: name: Test runs-on: [self-hosted, local, macOS, ARM64] + timeout-minutes: 45 steps: - name: Checkout uses: actions/checkout@v6 @@ -16,7 +17,10 @@ jobs: fetch-depth: 0 - name: Test - run: nix develop --command go test -race -count=1 ./... -tags=!examples + run: nix develop --command go test -race -count=1 ./... + + - name: Integration tests + run: nix develop --command bash -c "go list ./... | grep -v examples | xargs go test -count=1 -tags=integration -timeout=20m" release: name: Release @@ -55,6 +59,6 @@ jobs: sleep 5 LATEST_TAG=$(git describe --tags --abbrev=0 2>/dev/null || echo "") if [ -n "$LATEST_TAG" ]; then - echo "Warming Go proxy for github.com/jasoet/pkg/v2@${LATEST_TAG}" - GOPROXY=https://proxy.golang.org GO111MODULE=on nix develop --command go list -m "github.com/jasoet/pkg/v2@${LATEST_TAG}" || true + echo "Warming Go proxy for github.com/jasoet/pkg/v3@${LATEST_TAG}" + GOPROXY=https://proxy.golang.org GO111MODULE=on nix develop --command go list -m "github.com/jasoet/pkg/v3@${LATEST_TAG}" || true fi diff --git a/.gitignore b/.gitignore index 37b9832..c718bdc 100644 --- a/.gitignore +++ b/.gitignore @@ -229,4 +229,6 @@ temporal.db .worktrees/ # Node.js (semantic-release) -node_modules/ \ No newline at end of file +node_modules/ +# Superpowers workflow scratch (task briefs, reports, review diffs) +.superpowers/ diff --git a/.golangci.yml b/.golangci.yml index e981af5..cb3caf1 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,4 +1,4 @@ -# golangci-lint configuration for github.com/jasoet/pkg/v2 +# golangci-lint configuration for github.com/jasoet/pkg/v3 # Go utility library with focus on reliability, security, and performance version: "2" diff --git a/.releaserc.json b/.releaserc.json index 4d857b0..da7825c 100644 --- a/.releaserc.json +++ b/.releaserc.json @@ -1,6 +1,14 @@ { "branches": [ - "main" + "main", + { + "name": "next", + "prerelease": true + }, + { + "name": "release/v2", + "range": "2.13.x" + } ], "plugins": [ [ @@ -78,7 +86,7 @@ ] }, "writerOpts": { - "headerPartial": "## [{{version}}]({{host}}/{{owner}}/{{repository}}/compare/{{previousTag}}...{{currentTag}}) ({{date}})\n\n**Full Changelog**: {{host}}/{{owner}}/{{repository}}/compare/{{previousTag}}...{{currentTag}}\n\n```\ngo get github.com/jasoet/pkg/v2@{{currentTag}}\n```" + "headerPartial": "## [{{version}}]({{host}}/{{owner}}/{{repository}}/compare/{{previousTag}}...{{currentTag}}) ({{date}})\n\n**Full Changelog**: {{host}}/{{owner}}/{{repository}}/compare/{{previousTag}}...{{currentTag}}\n\n```\ngo get github.com/jasoet/pkg/v3@{{currentTag}}\n```" } } ], diff --git a/AI_PATTERN.md b/AI_PATTERN.md index 5197b26..da3dd27 100644 --- a/AI_PATTERN.md +++ b/AI_PATTERN.md @@ -1,15 +1,15 @@ # AI Pattern Guide -Guide for AI assistants working on projects that **use** `github.com/jasoet/pkg/v2`. This file is an index — read the linked READMEs and examples for full details. +Guide for AI assistants working on projects that **use** `github.com/jasoet/pkg/v3`. This file is an index — read the linked READMEs and examples for full details. ## Quick Start ```go -import "github.com/jasoet/pkg/v2/" +import "github.com/jasoet/pkg/v3/" ``` **Go Version:** 1.26+ (generics required) -**Install:** `go get github.com/jasoet/pkg/v2@latest` +**Install:** `go get github.com/jasoet/pkg/v3@latest` **v1 (no OTel):** `go get github.com/jasoet/pkg@v1.6.0` — preserved on [`release/v1`](https://github.com/jasoet/pkg/tree/release/v1) branch, unmaintained. **Project Template:** See [PROJECT_TEMPLATE.md](PROJECT_TEMPLATE.md) for recommended project structure, wiring patterns, test tiers (E2E), Swagger/OpenAPI setup, and Taskfile targets. @@ -80,7 +80,6 @@ cfg, err := config.LoadString[AppConfig](yamlContent, "APP") |---------|-------------|--------|----------| | [otel](./otel/) | OpenTelemetry unified config (tracing, metrics, logging) | [README](otel/README.md) | [examples_test.go](otel/examples_test.go), [instrumentation_example_test.go](otel/instrumentation_example_test.go) | | [config](./config/) | Type-safe YAML config with env overrides and validation | [README](config/README.md) | [examples/](examples/config/) | -| [logging](./logging/) | Structured logging with zerolog + OTel LoggerProvider | [README](logging/README.md) | [examples/](examples/logging/) | | [db](./db/) | Multi-database (PostgreSQL, MySQL, MSSQL) with GORM + migrations | [README](db/README.md) | [examples/](examples/db/) | | [docker](./docker/) | Container executor with dual API (functional + struct) | [README](docker/README.md) | [examples/](examples/docker/) | | [server](./server/) | HTTP server with Echo, health checks, graceful shutdown | [README](server/README.md) | [examples/](examples/server/) | @@ -99,11 +98,13 @@ cfg, err := config.LoadString[AppConfig](yamlContent, "APP") ### Connect to a Database ```go -pool, _ := db.ConnectionConfig{ - DBType: db.Postgresql, Host: "localhost", Port: 5432, - Username: "user", Password: "pass", DBName: "mydb", - OTelConfig: otelConfig, -}.Pool() +pool, _ := db.NewPool( + db.WithConnectionConfig(db.ConnectionConfig{ + DBType: db.Postgresql, Host: "localhost", Port: 5432, + Username: "user", Password: "pass", DBName: "mydb", + }), + db.WithOTelConfig(otelConfig), +) ``` > [db/README.md](db/README.md) for migrations, multi-DB, connection pooling. @@ -111,8 +112,13 @@ pool, _ := db.ConnectionConfig{ ### Start an HTTP Server ```go -cfg := server.DefaultConfig(8080, operation, shutdown) -server.StartWithConfig(cfg) +srv, _ := server.New( + server.WithPort(8080), + server.WithOperation(operation), + server.WithShutdown(shutdown), +) +go func() { <-shutdownSignal; _ = srv.Shutdown(context.Background()) }() +err := srv.Start() // blocks until Shutdown ``` > [server/README.md](server/README.md) for health checks, middleware, EchoConfigurer. @@ -120,7 +126,7 @@ server.StartWithConfig(cfg) ### Add Retry Logic ```go -cfg := retry.DefaultConfig().WithName("db.connect").WithOTel(otelConfig) +cfg := retry.New(retry.WithName("db.connect"), retry.WithOTelConfig(otelConfig)) err := retry.Do(ctx, cfg, func(ctx context.Context) error { return db.Ping(ctx) }) ``` diff --git a/CONTEXT.md b/CONTEXT.md new file mode 100644 index 0000000..ce46297 --- /dev/null +++ b/CONTEXT.md @@ -0,0 +1,69 @@ +# pkg + +A Go utility library of independent packages, each wrapping one concern (HTTP, gRPC, +database, containers, workflows) with OpenTelemetry instrumentation built in. Consumers +import only the packages they need. + +## Language + +### Package kinds + +**Utility package**: +A package whose wrapped dependency is incidental to why a consumer imports it — `rest`, +`config`, `db`, `docker`, `server`, `grpc`, `compress`, `concurrent`, `retry`, `ssh`, +`base32`. Third-party types are hidden from public signatures. +_Avoid_: wrapper package, helper package + +**SDK-integration package**: +A package whose whole purpose is to make a specific vendor SDK easier to use — `temporal` +and `argo`. Vendor types appear in public signatures deliberately, because the consumer is +writing against that SDK anyway. +_Avoid_: leaky package, thin wrapper + +**Selective de-leak**: +The decision to hide a third-party type behind a library-owned one in a utility package +while deliberately keeping it visible in an SDK-integration package. See +[ADR 0004](docs/adr/0004-selective-de-leak-of-third-party-types.md). +_Avoid_: abstraction, encapsulation + +**Escape hatch**: +A public method that deliberately returns a third-party type inside an otherwise de-leaked +package, because no library-owned shape would carry the same information. Documented as +such, never an oversight. +_Avoid_: leak, loophole, backdoor + +### Conventions + +**Convention contract**: +The set of rules every configurable package must satisfy — functional options constructor, +`OTelConfig` field with the non-serialized tags, `WithOTelConfig` option, instrumentation +through `otel.Layers`, and `Example*` tests behind README snippets. +_Avoid_: standard, style guide + +**Convention test**: +A test in `internal/archtest` that enforces the convention contract mechanically, by +reflection over registered config structs and by compile-time assignment of each package's +`WithOTelConfig`. Adding a package means extending the registry. +_Avoid_: architecture test, lint rule + +**OTel injection point**: +`WithOTelConfig(*otel.Config)` — the single supported way to give a package its telemetry +providers. See [ADR 0002](docs/adr/0002-otel-config-is-injected-never-serialized.md). +_Avoid_: otel setup, telemetry config + +**Docs-of-record**: +An `Example*` test that a README snippet is copied from, so documentation cannot drift from +a compiling API. A README code block without one is not trusted. +_Avoid_: sample, snippet test + +### Release lines + +**v3 line**: +Work on the `next` branch, published as `v3.0.0-next.N` prereleases, merged to `main` as +`v3.0.0`. The only line receiving features. See +[ADR 0001](docs/adr/0001-freeze-v2-and-ship-v3-as-one-big-bang.md). + +**Frozen line**: +`release/v2`, pinned at v2.13.1, open to emergency patches only. `release/v1` is closed +entirely at v1.6.0. +_Avoid_: legacy, deprecated, maintenance branch diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 0000000..8108816 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,143 @@ +# Contributing + +Thanks for your interest in the project. This guide covers the setup, the conventions, and +what a change has to satisfy before it can merge. + +## Development environment + +All dev tooling comes from the Nix flake; `task` wraps every command in `nix develop -c`, so +you never need to enter a shell manually. + +**Prerequisites:** + +- **Nix** with flakes enabled — [Determinate Nix Installer](https://install.determinate.systems/nix) +- **go-task** — `brew install go-task` (global, deliberately not in the flake) +- **Docker or Podman** — for integration tests. Either works; the Taskfile detects which. +- **gh** (optional) — for PRs and CI status + +```bash +git clone https://github.com/jasoet/pkg.git +cd pkg + +task nix:check # verify the toolchain +task docker:check # verify a container runtime +task test +``` + +Run `task --list` to see everything available. **Use `task ` rather than raw `go` +commands** — the tasks carry the flags and environment that CI uses, so "works locally" +means something. + +## Making a change + +1. **Branch off `next`.** v3 development happens there; `main` carries the released state. + Name branches `feat/...`, `fix/...`, `docs/...`, `ci/...`. + +2. **Write the test first.** Every behaviour change needs a test that failed before the fix. + Unit tests carry no build tag, integration tests use `//go:build integration`. + +3. **Verify before you push:** + + ```bash + task check # tests + lint + go vet -tags='example integration argo' ./... # tagged code CI also compiles + ``` + + The tagged vet matters: `task check` only compiles untagged code, and example- or + integration-only breakage has slipped past that gap before. + +4. **Open a PR against `next`** and let CI finish. Merges are squash-only, so the PR title + becomes the commit message — write it as a proper Conventional Commit. + +## Conventions + +### Commit messages + +[Conventional Commits](https://www.conventionalcommits.org/) — they drive releases, so the +type is not cosmetic: + +``` +(): + +[optional body] + +[optional footer] +``` + +Types: `feat`, `fix`, `docs`, `refactor`, `test`, `chore`, `perf`, `ci`, `style`. +Scope is the package (`fix(db): ...`). + +**Breaking changes need a `BREAKING CHANGE:` footer** *and* an entry in +[MIGRATION.md](./MIGRATION.md). Several v3 breaks shipped in `fix:`-typed commits without a +footer and would never have reached the release notes — the migration guide is the backstop, +so update it in the same PR as the break. + +### Package conventions + +Every configurable package must: + +1. Expose a functional-options constructor — `New(opts ...Option) (T, error)` where + construction can fail. +2. Carry `OTelConfig *otel.Config` tagged exactly `yaml:"-" mapstructure:"-"`. +3. Expose `WithOTelConfig(cfg *otel.Config) Option` as the single OTel injection point. +4. Instrument through `otel.Layers.Start*()` at layer boundaries. +5. Back README snippets with `Example*` tests, so documentation cannot drift from a + compiling API. +6. Use testify (`assert`/`require`). + +These are enforced mechanically by `internal/archtest` — **adding a package means extending +its registry**, or the convention test fails. See [CONTEXT.md](./CONTEXT.md) for the +vocabulary and [docs/adr/](./docs/adr/) for why the conventions are shaped this way. + +### Documentation + +Docs are treated as part of the product, not an afterthought. When a change affects the +public API, update in the same PR: + +- the package's `README.md` +- `MIGRATION.md`, if consumers must do something +- `INSTRUCTION.md` / `README.md`, for structural changes +- `AI_PATTERN.md`, which indexes consumer patterns + +Do not write a README snippet you have not compiled. + +### Adding a package + +1. Create `newpkg/`, `newpkg/README.md`, `newpkg/newpkg.go`, `newpkg/newpkg_test.go`. +2. Follow the six package conventions above. +3. Register it in `internal/archtest`. +4. Add an example under `examples/newpkg/` behind `//go:build example`. +5. Update the `README.md` package table and `AI_PATTERN.md`. + +## Testing + +| Tier | Build tag | Command | +|---|---|---| +| Unit | none | `task test` | +| Integration | `integration` | `task test:integration` (needs Docker/Podman) | +| Argo | `argo` | `task test:argo` (needs a k8s cluster with Argo) | +| Examples | `example` | `go build -tags=example ./...` | +| Everything | — | `task test:complete` | + +Integration tests use testcontainers with a 15-minute timeout; always +`defer container.Terminate(ctx)`. + +## Git authorship + +**Commits must be authored solely by a human contributor.** Do not add AI tools as +co-authors, committers, or contributors — no `Co-authored-by`, `Generated by`, or +`Created with` trailers, and no AI attribution in commit messages, PR descriptions, or code +comments. This applies to every commit, including those produced with tool assistance. + +## Versioning + +- **v3** (`next` branch) — the only line receiving features. +- **v2** (`release/v2`) — frozen at v2.13.1, emergency patches only. +- **v1** (`release/v1`) — closed at v1.6.0. + +See [ADR 0001](./docs/adr/0001-freeze-v2-and-ship-v3-as-one-big-bang.md) for the reasoning. + +## Reporting issues + +Include the package, the version, a minimal reproduction, and what you expected instead. +For security issues, please report privately rather than opening a public issue. diff --git a/INSTRUCTION.md b/INSTRUCTION.md index e5b8f9a..9cfb6a1 100644 --- a/INSTRUCTION.md +++ b/INSTRUCTION.md @@ -5,12 +5,13 @@ ## Project Overview -Production-ready Go utility library (v2) with OpenTelemetry instrumentation. 15 packages: otel, config, logging, db, docker, server, grpc, rest, concurrent, temporal, ssh, compress, argo, retry, base32. +Production-ready Go utility library (v3) with OpenTelemetry instrumentation. 14 packages: otel, config, db, docker, server, grpc, rest, concurrent, temporal, ssh, compress, argo, retry, base32. (The former `logging` package was merged into `otel` during the v3 rework.) -**Module Path:** `github.com/jasoet/pkg/v2` +**Module Path:** `github.com/jasoet/pkg/v3` **Go Version:** 1.26+ (uses generics) -**Test Coverage:** 79% +**Test Coverage:** Reported per package in `README.md`; regenerate with `task test:complete`. **v1 Branch:** [`release/v1`](https://github.com/jasoet/pkg/tree/release/v1) — final v1 release (v1.6.0), no longer maintained. Use `go get github.com/jasoet/pkg@v1.6.0` for projects that don't need OpenTelemetry. +**v3 Development:** v2 is frozen at v2.13.1 (`release/v2` branch, emergency patches only). v3 work happens on the `next` branch (prereleases `v3.0.0-next.N` (until the first BREAKING CHANGE commit lands on next, prereleases version from the last tag — e.g. 2.14.0-next.1)). Backlog: `docs/plans/2026-07-22-v3-audit-backlog.md`. Consumer-facing breaks go in `MIGRATION.md` **as they land** — several v3 breaks shipped in `fix:`-typed commits without `BREAKING CHANGE` footers and would otherwise never reach the release notes. ## ABSOLUTE RULE — Git Authorship @@ -38,12 +39,14 @@ attribute commits to AI. This applies to ALL commits, including those made by to | Path | Purpose | |------|---------| -| `/` | Package source (15 packages at root level) | +| `/` | Package source (14 packages at root level) | | `/README.md` | Per-package documentation | | `examples//` | Per-package runnable examples (`//go:build example`) | | `/*_test.go` | Unit tests (no build tag) | | `/*_integration_test.go` | Integration tests (`//go:build integration`) | | `docs/plans/` | Design docs and implementation plans | +| `docs/adr/` | Architecture decisions — the v3 shape, and why | +| `internal/archtest/` | Convention-enforcement tests — extend registry when unifying a package | | `.claude/` | Claude Code hooks and settings | | `flake.nix` | Nix flake — dev tool declarations | | `.envrc` | direnv auto-activation (optional) | @@ -53,6 +56,8 @@ attribute commits to AI. This applies to ALL commits, including those made by to | `AGENTS.md` | Byte-copy of CLAUDE.md (Kimi Code auto-load) | | `AI_PATTERN.md` | AI library consumer patterns index | | `PROJECT_TEMPLATE.md` | New project scaffolding guide | +| `MIGRATION.md` | v2 → v3 consumer migration guide — update when a break lands on `next` | +| `CONTEXT.md` | Domain glossary (package kinds, conventions, release lines) | | `README.md` | Human documentation | ## Taskfile Commands diff --git a/MAINTAINING.md b/MAINTAINING.md index b5f506c..1e1ff6c 100644 --- a/MAINTAINING.md +++ b/MAINTAINING.md @@ -1,82 +1,135 @@ # Maintaining Guide -This document explains how to maintain and release this library. +How this library is branched, released, and versioned. For contributing changes, see +[CONTRIBUTING.md](./CONTRIBUTING.md). ## Branch Strategy -### `main` - Active Development -- **Module Path:** `github.com/jasoet/pkg/v2` -- **Purpose:** Active development for v2.x releases -- **Go Version:** 1.26+ +| Branch | Module path | Status | Releases | +|---|---|---|---| +| `main` | `github.com/jasoet/pkg/v3` | Released v3 line | `v3.x.y` | +| `next` | `github.com/jasoet/pkg/v3` | v3 development | `v3.x.y-next.N` prereleases | +| `release/v2` | `github.com/jasoet/pkg/v2` | Frozen at v2.13.1 | `2.13.x` emergency patches only | +| `release/v1` | `github.com/jasoet/pkg` | Closed at v1.6.0 | none | + +Day-to-day work targets **`next`**, not `main`. Feature and fix branches are cut from +`next` and squash-merged back into it; each merge publishes a `v3.x.y-next.N` prerelease. +`main` only moves when a prerelease line is promoted (see below). + +Go's module-path versioning is what makes this work: `/v2` and `/v3` are different modules, +so consumers can import both while migrating. See +[ADR 0001](./docs/adr/0001-freeze-v2-and-ship-v3-as-one-big-bang.md). ## Releasing -Releases are fully automated via [semantic-release](https://github.com/semantic-release/semantic-release) on every push to `main`. +Releases are automated by [semantic-release](https://github.com/semantic-release/semantic-release) +on every push to `main`, `next`, and `release/v2`. ### What triggers a release -| Commit Type | Release | Example | +| Commit type | Bump | Example | |---|---|---| -| `feat` | Minor (v2.x.0) | `feat(server): add gRPC interceptor` | -| `fix` | Patch (v2.0.x) | `fix(compress): handle empty input` | -| `perf` | Patch | `perf(db): reduce query allocations` | -| `refactor` | Patch | `refactor(otel): simplify provider setup` | -| Breaking change | Major (vX.0.0) | `feat!: remove deprecated API` or footer `BREAKING CHANGE:` | +| `feat` | minor | `feat(server): add gRPC interceptor` | +| `fix` | patch | `fix(compress): handle empty input` | +| `perf` | patch | `perf(db): reduce query allocations` | +| `refactor` | patch | `refactor(otel): simplify provider setup` | +| Breaking | major | `feat(api)!: remove deprecated method`, or a `BREAKING CHANGE:` footer | -### What does NOT trigger a release +`docs`, `test`, `ci`, `chore`, `style` and `build` never trigger a release. -`docs`, `test`, `ci`, `chore`, `style`, `build` commits are excluded. +### Normal changes: squash merge into `next` -### Workflow +The PR title becomes the commit message, so it must be a valid Conventional Commit. Write +the PR description carefully — it becomes the release notes. -1. Merge PR to `main` with conventional commit title -2. CI runs tests -3. semantic-release analyzes commits since last tag -4. If a release is warranted, it creates a GitHub release with notes -5. Go module proxy is warmed automatically +**A breaking change needs both** a `BREAKING CHANGE:` footer *and* an entry in +[MIGRATION.md](./MIGRATION.md). Do not rely on the footer alone: several v3 breaks shipped +in `fix:`-typed commits without footers and never reached the generated notes. The +migration guide is the backstop. -## CI Pipelines +### Promoting a line: merge commit into `main`, never squash -- **`ci.yml`** - Runs on PRs: test (with race detector) + lint -- **`release.yml`** - Runs on push to `main`: test + semantic-release +**When merging `next` into `main`, use a merge commit.** -## Conventional Commits +```bash +gh pr merge --merge # correct +gh pr merge --squash # WRONG — silently produces the wrong version +``` -All PR titles must follow [Conventional Commits](https://www.conventionalcommits.org/): +Squashing collapses the whole line into a single commit and **destroys every +`BREAKING CHANGE` footer in it**. semantic-release then analyses one commit against the last +tag on `main` and computes a bump from that alone. -``` -(): +This is measured, not theoretical. Simulating both merges of the v3 line locally and running +`semantic-release --dry-run`: -[optional body] +| Merge strategy | Surviving `BREAKING CHANGE` footers | Computed version | +|---|---|---| +| `--merge` | 22 | **3.0.0** | +| `--squash` | **0** | **2.14.0** | + +A `v2.14.0` tag on a module whose path is `/v3` is not installable — and the mistake is only +visible after the tag is published. + +You can re-run that check before any promotion, without pushing anything: -[optional footer(s)] +```bash +git checkout main && git reset --hard origin/main +git merge --no-ff --no-edit origin/next +GITHUB_TOKEN=$(gh auth token) bunx semantic-release --dry-run --no-ci +git reset --hard origin/main # discard the simulation ``` -### Best Practices for PR Authors -- Write detailed PR descriptions (they become release notes when squash-merged) -- Use conventional commit format in PR title -- Include scope when the change targets a specific package +A merge commit also produces complete release notes, since every `feat`/`fix` on the line +stays individually attributed. + +### Pre-promotion checklist + +1. `task ci:check` and `go vet -tags='example integration argo' ./...` clean on `next`. +2. Integration suite green: `task test:integration`. +3. `MIGRATION.md` covers every break on the line, including any that shipped without a + footer. +4. Package coverage figures in `README.md` regenerated. +5. Open the PR `next` → `main` and confirm the **API compatibility check reports + informationally, not blocking** — `ci.yml` keys that off `head_ref == 'next'`. + gorelease reporting `Inferred base version: none` is expected until the first + non-prerelease tag exists on the new major. +6. Merge with `--merge`, then confirm the published tag is what you expected before + announcing anything. + +## CI Pipelines + +| Workflow | Triggers | Does | +|---|---|---| +| `ci.yml` | push to `main`/`next`/`release/v2` and tags; PRs targeting them | lint, race tests, `go vet` over build-tagged code, gorelease API check | +| `release.yml` | push to `main`/`next`/`release/v2`; manual dispatch | race tests, integration tests, semantic-release, Go proxy warmup | + +Both run on the self-hosted `[self-hosted, local, macOS, ARM64]` runner. + +The gorelease API check is **blocking** everywhere except where `next` is involved +(`ref_name`, `base_ref`, or `head_ref` equal to `next`), because breaking changes are the +point of the v3 line. It is pinned to the `golang.org/x/exp` pseudo-version in `go.mod`; +bump both together. + +`ci.yml` runs `go vet` with `-tags='example integration argo'` as a separate step. `task +check` compiles only untagged code, and example- or integration-only breakage has slipped +through that gap before. ## Import Paths ```go -import "github.com/jasoet/pkg/v2/compress" -import "github.com/jasoet/pkg/v2/server" +import "github.com/jasoet/pkg/v3/server" ``` ```bash -go get github.com/jasoet/pkg/v2@latest +go get github.com/jasoet/pkg/v3@latest ``` -## Testing - -Before any release: - -1. **Unit Tests:** `task test` -2. **Integration Tests:** `task test:integration` -3. **Linting:** `task lint` +## Testing Before a Release -Or run everything: ```bash -task test:complete # Runs all tests with coverage +task test # unit +task test:integration # integration (Docker or Podman required) +task lint +task test:complete # everything, including argo (needs a k8s cluster) ``` diff --git a/MIGRATION.md b/MIGRATION.md new file mode 100644 index 0000000..7a87899 --- /dev/null +++ b/MIGRATION.md @@ -0,0 +1,578 @@ +# Migrating from v2 to v3 + +v3 is a single big-bang release. It unifies conventions across all packages, removes +the `logging` package, and de-leaks selected third-party types from public signatures. + +**Everything in this guide is a compile-time or behavioural break.** Work through the +[module path](#1-module-path) change first — it is mechanical — then only the sections +for packages you actually import. + +- **v2 is frozen at v2.13.1** on the `release/v2` branch. It receives emergency patches + only. There is no deprecation window; v2 and v3 can be imported side by side during a + migration because their module paths differ. +- **v1** remains available at `github.com/jasoet/pkg@v1.6.0` for projects that do not + want OpenTelemetry. + +## Contents + +- [1. Module path](#1-module-path) +- [2. `logging` package removed](#2-logging-package-removed) +- [3. Conventions that changed everywhere](#3-conventions-that-changed-everywhere) +- [Per-package migration](#per-package-migration) + - [otel](#otel) · [config](#config) · [db](#db) · [docker](#docker) · [server](#server) + - [grpc](#grpc) · [rest](#rest) · [retry](#retry) · [temporal](#temporal) · [argo](#argo) + - [ssh](#ssh) · [compress](#compress) · [concurrent](#concurrent) · [base32](#base32) +- [4. Telemetry changes that need no code edit](#4-telemetry-changes-that-need-no-code-edit) + +--- + +## 1. Module path + +The module path is now `github.com/jasoet/pkg/v3`. + +```bash +go get github.com/jasoet/pkg/v3@v3.0.0 + +# rewrite imports across the tree +find . -name '*.go' -not -path './vendor/*' \ + -exec sed -i '' 's|github.com/jasoet/pkg/v2|github.com/jasoet/pkg/v3|g' {} + + +go mod tidy +``` + +On Linux use `sed -i` without the `''`. + +Do this first and in its own commit — the remaining sections assume it is done, and +mixing a path rewrite with semantic changes makes the diff unreviewable. + +## 2. `logging` package removed + +`logging` existed only to serve `otel`, and the dependency ran backwards. It is now part +of `otel` with identical signatures, so this is an import-and-qualifier change only: + +| v2 | v3 | +|---|---| +| `logging.Initialize` | `otel.Initialize` | +| `logging.InitializeWithFile` | `otel.InitializeWithFile` | +| `logging.ContextLogger` | `otel.ContextLogger` | +| `logging.LogLevel` | `otel.LogLevel` | + +```go +// v2 +import "github.com/jasoet/pkg/v2/logging" +err := logging.Initialize("my-service", false) +log := logging.ContextLogger(ctx, "handler") + +// v3 +import "github.com/jasoet/pkg/v3/otel" +err := otel.Initialize("my-service", false) +log := otel.ContextLogger(ctx, "handler") +``` + +## 3. Conventions that changed everywhere + +v3 settles on one shape for configurable packages. If you have wrapped these packages, +expect the same three edits in each: + +1. **Functional options replace mutating builders and config structs.** + `New(opts ...Option) (T, error)` where construction can fail. +2. **`WithOTelConfig(*otel.Config)` is the single OTel injection point.** `retry`'s + `WithOTel` was the odd one out and is renamed. +3. **`OTelConfig` is never serialized.** It is tagged `yaml:"-" mapstructure:"-"` on every + config struct, so it must be injected via code, never loaded from YAML. + +`internal/archtest` enforces all three mechanically, so they will not drift back. + +--- + +# Per-package migration + +## otel + +**Config construction is options-based.** The mutating builder methods are gone. + +```go +// v2 — mutating builders on *Config +cfg := otel.NewConfig("my-service"). + WithServiceVersion("1.0.0"). + WithTracerProvider(tp). + DisableMetrics() + +// v3 — package-level options +cfg := otel.NewConfig("my-service", + otel.WithServiceVersion("1.0.0"), + otel.WithTracerProvider(tp), + otel.WithoutMetrics(), +) +``` + +`DisableTracing`/`DisableMetrics` are renamed `WithoutTracing`/`WithoutMetrics` +(and `WithoutLogging` joins them) — "Disable" read like an imperative action on an +already-built config, which is exactly the mutation model being removed. + +**`WithOTLPEndpoint("")` now returns an error** instead of silently disabling OTLP +export. If you were passing a possibly-empty endpoint from configuration, branch on it: + +```go +opts := []otel.LoggerProviderOption{otel.WithConsoleOutput(true)} +if endpoint != "" { + opts = append(opts, otel.WithOTLPEndpoint(endpoint, insecure)) +} +``` + +Silently exporting nothing because an env var was unset is the failure mode this +prevents — it is worth the explicit branch. + +**Nil-config zerolog fallback** now defaults to `Info` (was unset) and labels the emitter +as `scope` rather than `service`. Log-parsing rules keyed on `service` need updating. + +**Duplicate span exception events are deduped** — a recorded error no longer appears twice +on the same span. + +## config + +`*viper.Viper` no longer leaks into public signatures. + +```go +// v2 +cfg, err := config.LoadStringWithConfig[MyConfig](yamlStr, func(v *viper.Viper) { + v.SetDefault("port", 8080) +}) + +// v3 +cfg, err := config.LoadStringWithOptions[MyConfig](yamlStr, + config.WithDefaults(map[string]any{"port": 8080}), + config.WithEnvPrefix("APP"), +) +``` + +- `LoadStringWithConfig` → `LoadStringWithOptions` +- `NestedEnvVars` → `config.WithNestedEnvVars(prefix, keyDepth, configPath)` + +`LoadString[T](s, envPrefix ...string)` is unchanged for the simple case. + +**`keyDepth` is prefix-relative.** It counts segments *after* the env prefix, not from the +start of the variable name. See `config/README.md` for the worked example — this is the +one parameter likely to be silently wrong after the move. + +## db + +**`(*ConnectionConfig).Pool()` is removed.** + +```go +// v2 +pool, err := cfg.Pool() + +// v3 +pool, err := db.NewPool(db.WithConnectionConfig(cfg)) +``` + +**Gorm migration wrappers are removed.** The API had four ways to do one thing. + +```go +// v2 +err := db.RunPostgresMigrationsWithGorm(ctx, gormDB, migrationsFS, ".") + +// v3 +sqlDB, err := gormDB.DB() +if err != nil { + return err +} +err = db.RunPostgresMigrations(ctx, sqlDB, migrationsFS, ".") +``` + +`RunPostgresMigrationsDownWithGorm` → `RunPostgresMigrationsDown` the same way. + +**No code change, but watch your dashboards:** pool metrics were gated behind +`IsTracingEnabled()`, so a metrics-only config emitted none. Fixed — a metrics-only +consumer will now start emitting `db.client.connections.*` series that never appeared +before. Alerts with "no data" conditions on those series may fire on first deploy. + +**`RedactedDsn` is now structural** rather than a naive string replacement, so a password +that also appeared as a substring elsewhere in the DSN no longer leaks. Output changes only +for those pathological cases, and only in the safe direction. + +## docker + +**`WaitStrategy` no longer takes a docker client.** This is the selective de-leak: your +custom strategies stop depending on `docker/docker` types. + +```go +// v2 +func (s MyStrategy) WaitUntilReady(ctx context.Context, cli *client.Client, containerID string) error + +// v3 +func (s MyStrategy) WaitUntilReady(ctx context.Context, target docker.ContainerTarget) error +``` + +`WaitForFunc` changes the same way. `ContainerTarget` exposes `State(ctx)`, `Logs(ctx)` +and the container ID. + +**Limitation to plan for:** exec-based readiness checks (e.g. `pg_isready` via +`ContainerExec`) are no longer expressible through `WaitForFunc`, because `ContainerTarget` +has no exec capability. Such consumers must construct their own client for now. An +Exec-capable target is under consideration for v3.x. `ContainerTarget.Logs()` also +hardcodes `Follow`/`Timestamps` off. + +**Renames and removals:** + +| v2 | v3 | +|---|---| +| `Executor.WaitForHealthy` | `Executor.WaitHealthy` | +| `NatPort`, `PortBindings`, `ExposedPorts` | removed (unused helpers) | +| `LogEntry.Timestamp` | removed (never populated) | + +**`Executor.Inspect()` and `Executor.GetStats()` still return `docker/docker` types.** This +is deliberate — see [ADR 0004](docs/adr/0004-selective-de-leak-of-third-party-types.md). +They are documented escape hatches, not an oversight. + +## server + +This package changed the most. **Signal handling was removed** — that is the change most +likely to break a production deployment silently, so start there. + +```go +// v2 — blocked until SIGINT/SIGTERM, then drained gracefully +server.Start(server.Config{Port: 8080, ...}) + +// v3 — you own the lifecycle, and therefore the signal handling +srv, err := server.New( + server.WithPort(8080), + server.WithOTelConfig(otelCfg), +) +if err != nil { + return err +} + +go func() { + if err := srv.Start(); err != nil { + log.Error().Err(err).Msg("server stopped") + } +}() + +sigCh := make(chan os.Signal, 1) +signal.Notify(sigCh, os.Interrupt, syscall.SIGTERM) +<-sigCh + +ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) +defer cancel() +return srv.Shutdown(ctx) +``` + +**If you skip the `signal.Notify` block your service loses graceful termination** and will +drop in-flight requests on deploy. Nothing will fail to compile to tell you. + +Removed: `Start`, `StartWithConfig`, `DefaultConfig` package functions. + +Other behavioural changes: + +- **Port validation now fails at `New`**, not at start. +- **Restart after shutdown returns an error.** A `Server` is single-use. +- **`Shutdown` is idempotent** — calling it twice is safe. +- **`WithOTelConfig` now auto-installs Echo OTel middleware**, so `http.server.*` spans and + metrics appear for consumers who previously passed a config and got only partial + instrumentation. New series, no code change. +- A doc comment claiming health endpoints were unauthenticated was **wrong** and is + corrected; behaviour is unchanged (the package's own test always disproved it). + +## grpc + +**Ten dead or misleading exported symbols were removed.** All had zero non-test callers — +the `Server` wires the gateway and health endpoints itself: + +`SetupGatewayForH2C`, `SetupGatewayForSeparate`, `GatewayRoute`, +`MountGatewayWithStripPrefix`, `GatewayHealthMiddleware`, `LogGatewayRoutes`, +`CreateHealthHandlers`, `EchoHealthCheckMiddleware`, `CreateEchoHealthHandler`, +`RegisterEchoIndividualHealthChecks`. + +**Behavioural changes that shipped without a `BREAKING CHANGE` footer** — these will not +appear in the generated release notes, so they are listed here deliberately: + +1. **`MountGatewayOnEcho` now strips the base path.** The mux sees proto http-rule paths + verbatim. If you registered mux patterns *including* the prefix, switch to + proto-relative ones: + + ```go + // v2: pattern had to include the base path + mux.HandlePath("GET", "/api/v1/users", handler) + + // v3: base path is stripped before the mux sees the request + mux.HandlePath("GET", "/users", handler) + ``` + +2. **`Start`/`StartH2C`/`StartSeparate` return `nil` on clean shutdown**, not + `http.ErrServerClosed`. Drop the special-casing: + + ```go + // v2 + if err := srv.Start(); err != nil && !errors.Is(err, http.ErrServerClosed) { + return err + } + + // v3 + if err := srv.Start(); err != nil { + return err + } + ``` + +3. **`GetGRPCServer()` returns nil after `Stop`**, until the next `Start`. The server is + rebuilt per start cycle; a long-lived cached reference will be stale. + +**Fixed bugs** (no action needed): a restarted server could not be stopped, the `running` +flag was sticky, and H2C timeouts were applied to gRPC streams. + +Note the deliberate divergence from `server`: **grpc supports `Start`→`Stop`→`Start` +cycles, `server` does not.** See [ADR 0005](docs/adr/0005-lifecycle-divergence-between-server-and-grpc.md). + +## rest + +**`*resty.Response` no longer leaks.** `MakeRequest`/`MakeRequestWithTrace` return +`*rest.Response`: + +```go +// v2 +resp, err := client.MakeRequest(ctx, "GET", url, nil, nil) +body := resp.Body() // resty +code := resp.StatusCode() + +// v3 +resp, err := client.MakeRequest(ctx, "GET", url, nil, nil) +body := resp.Body() // rest.Response +code := resp.StatusCode() +``` + +The shape is close enough that most call sites compile unchanged; the type in your own +signatures is what needs editing. + +**Status helpers moved onto `Response`,** and one was misnamed: + +| v2 (package func) | v3 (method) | +|---|---| +| `rest.IsUnauthorized(resp)` | `resp.IsAuthError()` | +| `rest.IsNotFound(resp)` | `resp.IsNotFound()` | + +`IsUnauthorized` folded 403 into 401, so the name was a lie — hence `IsAuthError`. + +**`RequestInfo.TraceInfo` is now `rest.TraceInfo`** (was resty's). + +**Error constructors are unexported:** `NewUnauthorizedError`, `NewExecutionError`, +`NewServerError`, `NewResponseError`, `NewResourceNotFoundError` and `RecordRetry`. The +error *types* remain exported, so type switches and `errors.As` keep working. + +**`Client.HandleResponse` was unexported** (this also shipped without a footer). Typed +errors for non-2xx responses now come from `MakeRequest`/`MakeRequestWithTrace` directly, +and the returned `*rest.Response` is non-nil on HTTP errors, so status and body stay +inspectable. If you used `GetRestClient()` as an escape hatch and called `HandleResponse` +yourself, you must write your own status mapping. + +**Retry policy changed — this one is easy to miss:** + +- **POST and PATCH are no longer retried by default.** Only idempotent methods are. If you + relied on automatic POST retries, either make the endpoint idempotent (preferred) or + handle retries at the call site. +- `Retry-After` response headers are now honoured. +- `ExecutionError` and `UnauthorizedError` messages now include the cause / response body. + **Do not match on error strings** — use `errors.As` with the exported types. + +## retry + +```go +// v2 — builder methods +cfg := retry.NewConfig(). + SetOperationName("db.connect"). + SetMaxRetries(3). + WithOTel(otelCfg) + +// v3 — functional options +cfg := retry.New( + retry.WithName("db.connect"), + retry.WithMaxRetries(3), + retry.WithOTelConfig(otelCfg), +) +``` + +- `Config.OperationName` → `Config.Name` +- `WithOTel` → `WithOTelConfig` (aligning with every other package) +- **Invalid configuration now returns an error from `Do`** instead of panicking inside a + setter. Setters that panicked while exported fields went unguarded was an inconsistent + contract; validation now happens in one place. +- **`RandomizationFactor` accepts `[0, 1]`** — the range widened from `[0, 1)`, so `1.0` is + now valid. Strictly more permissive; nothing that worked stops working. + +## temporal + +`temporal` is an SDK-integration package: SDK types in signatures are by design +(see [ADR 0004](docs/adr/0004-selective-de-leak-of-third-party-types.md)). What changed is +the constructor shape and **client ownership**. + +```go +// v2 — interface{} constructors, manager owned the client +cfg := &temporal.Config{HostPort: "localhost:7233", Namespace: "default"} +wm, err := temporal.NewWorkerManager(cfg) +defer wm.Close() + +// v3 — typed options, caller owns the client +c, err := temporal.NewClient( + temporal.WithHostPort("localhost:7233"), + temporal.WithNamespace("default"), + temporal.WithOTelConfig(otelCfg), +) +if err != nil { + return err +} +defer c.Close() // <- you close it now + +wm, err := temporal.NewWorkerManager(c) +if err != nil { + return err +} +defer wm.Close(ctx) // <- takes ctx, does NOT close the client +``` + +`temporal.WithConfig(cfg)` accepts a whole `Config` if you already load one from YAML. + +**The ownership change is the dangerous one.** Managers now *borrow* a caller-owned +`client.Client`. If you previously relied on `wm.Close()` to close the client, you now leak +the connection unless you close it yourself. + +Also: + +- `Close()` → `Close(ctx)` on `WorkerManager` and `ScheduleManager`. +- **`WorkflowManager.Close` was removed entirely.** It had become a no-op. The commit's + `BREAKING CHANGE` footer omits this, so it would not otherwise reach the release notes. + `WorkflowManager` now has no `Close` — close the client you passed in instead. +- `NewWorkflowManagerWithNamespace`'s `namespace` parameter is **now always authoritative**. + It was previously ignored when a `*Config` was also passed — if you were relying on the + config value winning, you will now get the parameter's value. +- `NewClient`/`Close` accept a context. +- Namespace handling is consistent across the package; workflow history attribution and + TLS/auth support were fixed and added respectively. + +## argo + +Like `temporal`, an SDK-integration package by design. + +**`argo.Option` no longer returns an error** — no option ever failed: + +```go +// v2 +type Option func(*Config) error + +// v3 +type Option func(*Config) +``` + +**Operations no longer take a positional `*otel.Config`.** They read it from the context, +which is what `NewClient` returns: + +```go +// v2 +created, err := argo.SubmitWorkflow(ctx, client, wf, otelCfg) + +// v3 — NewClient returns the ctx carrying the OTel config; thread it through +ctx, client, err := argo.NewClientWithOptions(ctx, argo.WithOTelConfig(otelCfg)) +if err != nil { + return err +} +created, err := argo.SubmitWorkflow(ctx, client, wf) +``` + +**Thread the `ctx` returned by `NewClient` through to every operation.** Passing a fresh +`context.Background()` silently disables instrumentation — no error, just no telemetry. +This applies to `SubmitWorkflow`, `SubmitAndWait`, `GetWorkflowStatus`, `ListWorkflows` +and `DeleteWorkflow`. + +**Fixed:** `Namespace()` returned an untrimmed newline in in-cluster mode, which broke it +outright — if in-cluster mode never worked for you on v2, this is why. + +`SubmitAndWait`'s poll interval is now configurable via `argo.WithPollInterval(d)` +(was hardcoded at 5s), and timeout/failure errors are sentinels (`argo.ErrWaitTimeout`, +`argo.ErrWorkflowFailed`) usable with `errors.Is`. + +## ssh + +**Source-compatible for construction** — `New` is variadic — but several behaviours changed: + +- **`Close` now tears down in-flight forwarded connections immediately** instead of + draining them. Finish your work before calling `Close`. In exchange, `Close` no longer + blocks for ~90s against keep-alive clients. +- **Half-close propagation**: peers now see EOF promptly, which is observable for streaming + protocols. +- **`Close` error text is wrapped** as `"SSH client close error: %w"`. Use `errors.Is`/`As` + rather than string matching — the package's old error contract was string-based and the + README's matching guidance did not match any real error string. +- **Secrets are `yaml:"-"` by design**: `Password`, `PrivateKey` and `PrivateKeyPassphrase` + are never loaded from YAML. **A `password:` key in your YAML is silently dropped** — + inject secrets via env or code. If SSH auth breaks after upgrading, check this first. +- `Config` gains an `OTelConfig` field with `WithOTelConfig()` plumbing and + `otel.Layers` instrumentation. + +The accept loop no longer busy-spins on persistent errors, and `Start`→`Close`→`Start` +cycles no longer race. + +## compress + +- **`UnGz` now enforces `WithMaxArchiveSize`.** It was silently ignored. Extractions over + the limit fail with `ErrSizeLimitExceeded` — if you set a limit expecting it to apply to + gzip and it did not, extractions that used to succeed will now correctly fail. +- **Error message texts changed** for non-directory source/destination and tar guard-rail + rejections. They now wrap `ErrNotDirectory`, `ErrPathTraversal` and + `ErrSizeLimitExceeded` — match with `errors.Is`, not on strings. +- Symlink writes are refused, overwrites truncate, and the size cap is hard. + +## concurrent + +**`ExecuteConcurrentlyTyped` type parameters are now `[R, T]` (result first).** + +```go +// v2 +results, err := concurrent.ExecuteConcurrentlyTyped[Input, Output](ctx, funcs) + +// v3 +results, err := concurrent.ExecuteConcurrentlyTyped[Output, Input](ctx, funcs) +``` + +If you relied on inference this compiles unchanged. **If you specified the parameters +explicitly and both are the same type, it still compiles and is now wrong** — check these +call sites by hand. + +`ExecuteConcurrently` gains nil-`resultBuilder` validation and captures panic stacks. + +## base32 + +No API breaks. `AppendChecksum` and `ValidateChecksum` now normalize their input like the +other entry points, so dashed and lowercase input is accepted consistently rather than +rejected. Sentinel errors were added. + +The README and doc comments contained systematically wrong encoded values in v2 — if you +built expectations from those examples rather than from running the code, re-check them. + +--- + +## 4. Telemetry changes that need no code edit + +These require no source change but will alter what your observability backend receives. +Check dashboards and alerts after deploying: + +| Change | Effect | +|---|---| +| **`server` and `grpc` gateway now continue inbound W3C traces** | Requests carrying `traceparent` produce spans parented to the caller instead of new roots. Traces that appeared as separate roots will now join. | +| **`db` pool metrics no longer gated on tracing** | Metrics-only configs start emitting `db.client.connections.*`. | +| **`server` `WithOTelConfig` auto-installs Echo middleware** | New `http.server.*` spans and metrics. | +| **`ssh` gains OTel instrumentation** | New spans/metrics where there were none. | +| **`otel` dedupes span exception events** | Error counts derived from span events drop (they were double-counted). | +| **`rest` retry counter wired into the resty hook** | The retry metric was dead code in v2 and now reports real values. | + +`server` and the `grpc` gateway both emit metrics named `http.server.*` but with different +attribute sets — `server` uses method + status; the gateway adds `http.route` and +`active_requests`. If you aggregate across both, account for the difference. This is +documented rather than aligned; see [ADR 0005](docs/adr/0005-lifecycle-divergence-between-server-and-grpc.md). + +--- + +## Getting help + +- Per-package detail lives in each package's `README.md`. +- Runnable examples are under `examples//`. +- The full audit that produced this release: `docs/plans/2026-07-22-v3-audit-backlog.md`. +- Architectural decisions: `docs/adr/`. diff --git a/PROJECT_TEMPLATE.md b/PROJECT_TEMPLATE.md index 96ff0dc..2361aba 100644 --- a/PROJECT_TEMPLATE.md +++ b/PROJECT_TEMPLATE.md @@ -1,6 +1,6 @@ # PROJECT_TEMPLATE.md -Comprehensive guide for AI agents and developers scaffolding new Go projects that depend on `github.com/jasoet/pkg/v2`. +Comprehensive guide for AI agents and developers scaffolding new Go projects that depend on `github.com/jasoet/pkg/v3`. > **Audience:** AI code-generation agents (Claude, Cursor, Copilot) and human developers. > **Scope:** Consumer projects — applications built _with_ this library, not contributions _to_ it. @@ -313,16 +313,20 @@ Automatic via Viper: `APP_SERVER_PORT=9090` overrides `server.port`. For deeply nested structs, use: ```go -// keyDepth is the index of the entity-name token in the underscore-split env key, -// including prefix tokens. APP_DATABASE_USER_NAME → ["APP","DATABASE","USER","NAME"], -// so keyDepth=1 treats "DATABASE" as the entity under config path "database". -config.NestedEnvVars("APP", 1, "database", viperInstance) +// keyDepth is prefix-relative: the prefix is stripped, then keyDepth indexes the +// remaining underscore-split tokens. APP_DATABASE_USER_NAME → ["DATABASE","USER","NAME"], +// so keyDepth=0 treats "DATABASE" as the entity under config path "database". +cfg, err := config.LoadStringWithOptions[AppConfig](yamlContent, + config.WithEnvPrefix("APP"), + config.WithNestedEnvVars("APP", 0, "database"), +) ``` ### Layer 3: Runtime Functional Options ```go -pool, err := cfg.Database.Pool() // OTelConfig injected at runtime, not from YAML +// OTelConfig injected at runtime via db.WithOTelConfig, never from YAML +pool, err := db.NewPool(db.WithConnectionConfig(cfg.Database)) ``` **Rule:** `OTelConfig *otel.Config` fields must always use `yaml:"-" mapstructure:"-"` tags. Never serialize OTel config — inject it at runtime via functional options or direct assignment. @@ -334,20 +338,17 @@ pool, err := cfg.Database.Pool() // OTelConfig injected at runtime, not from YAM ### Bootstrap ```go -// Create OTel config with service name -otelCfg := otel.NewConfig("myapp") - -// Optionally attach real providers (nil = no-op, zero overhead) -otelCfg = otelCfg. - WithTracerProvider(tracerProvider). - WithMeterProvider(meterProvider) - -// For OTel-based logging (replaces zerolog global) +// Optional: OTel-based logging provider (console + optional OTLP export) loggerProvider, err := otel.NewLoggerProviderWithOptions("myapp", otel.WithConsoleOutput(true), - otel.WithLogLevel(otel.LogLevel("info")), + otel.WithLogLevel(otel.LogLevelInfo), ) -otelCfg = otelCfg.WithLoggerProvider(loggerProvider) + +// Create OTel config once; unattached providers stay no-op with zero overhead +otelCfg := otel.NewConfig("myapp", + otel.WithTracerProvider(tracerProvider), + otel.WithMeterProvider(meterProvider), + otel.WithLoggerProvider(loggerProvider)) // Store in context for downstream access ctx = otel.ContextWithConfig(ctx, otelCfg) @@ -356,9 +357,11 @@ ctx = otel.ContextWithConfig(ctx, otelCfg) ### Pass to Components ```go -// Database — direct field assignment -cfg.Database.OTelConfig = otelCfg -pool, err := cfg.Database.Pool() +// Database — functional option +pool, err := db.NewPool( + db.WithConnectionConfig(cfg.Database), + db.WithOTelConfig(otelCfg), +) // REST client — functional option client := rest.NewClient( @@ -366,10 +369,11 @@ client := rest.NewClient( rest.WithRestConfig(restCfg), ) -// Retry — builder method (value receiver) -retryCfg := retry.DefaultConfig(). - WithName("db.connect"). - WithOTel(otelCfg) +// Retry — functional options +retryCfg := retry.New( + retry.WithName("db.connect"), + retry.WithOTelConfig(otelCfg), +) ``` ### No-Op Pattern @@ -568,15 +572,17 @@ Services should never return HTTP-specific errors — keep the domain clean. ### Connection Pool ```go -pool, err := db.ConnectionConfig{ - DBType: db.Postgresql, - Host: cfg.Database.Host, - Port: cfg.Database.Port, - Username: cfg.Database.Username, - Password: cfg.Database.Password, - DBName: cfg.Database.DBName, - OTelConfig: otelCfg, // Automatic query tracing -}.Pool() +pool, err := db.NewPool( + db.WithConnectionConfig(db.ConnectionConfig{ + DBType: db.Postgresql, + Host: cfg.Database.Host, + Port: cfg.Database.Port, + Username: cfg.Database.Username, + Password: cfg.Database.Password, + DBName: cfg.Database.DBName, + }), + db.WithOTelConfig(otelCfg), // Automatic query tracing +) ``` > **TLS default:** `SSLMode` defaults to `"require"` for PostgreSQL and MSSQL. For local dev databases without TLS (e.g. the compose stack), add `SSLMode: "disable"` to your `ConnectionConfig` YAML. @@ -594,8 +600,14 @@ var FS embed.FS ``` ```go -// In main.go -err := db.RunPostgresMigrationsWithGorm(ctx, pool, migrations.FS, ".") +// In main.go — RunPostgresMigrations takes the underlying *sql.DB +sqlDB, err := pool.DB() +if err != nil { + log.Fatal(err) +} +if err := db.RunPostgresMigrations(ctx, sqlDB, migrations.FS, "."); err != nil { + log.Fatal(err) +} ``` **Migration file naming:** `{sequence}_{description}.{up|down}.sql` @@ -754,7 +766,7 @@ External API wrappers live in `internal/shared/client/`. Each client wraps a sin ### Client Pattern -Use `jasoet/pkg/v2/rest` for HTTP calls with automatic OTel instrumentation and retry support: +Use `jasoet/pkg/v3/rest` for HTTP calls with automatic OTel instrumentation and retry support: ```go // internal/shared/client/weather_client.go @@ -918,31 +930,42 @@ func (s *Service) ReportWithAudit(ctx context.Context, req ReportRequest) (*mode ### Starting the Server ```go -serverCfg := server.Config{ - Port: cfg.Server.Port, - ShutdownTimeout: cfg.Server.ShutdownTimeout, - Middleware: []echo.MiddlewareFunc{ +srv, err := server.New( + server.WithPort(cfg.Server.Port), + server.WithShutdownTimeout(cfg.Server.ShutdownTimeout), + server.WithMiddleware( middleware.Recover(), middleware.Logger(), - }, - EchoConfigurer: func(e *echo.Echo) { + ), + server.WithEchoConfigurer(func(e *echo.Echo) { // Register all routes here e.GET("/swagger/*", echoSwagger.WrapHandler) apiV1 := e.Group("/api/v1") userHandler.RegisterRoutes(apiV1.Group("/users")) - }, - Operation: func(e *echo.Echo) { + }), + server.WithOperation(func(e *echo.Echo) { // Additional startup operations - }, - Shutdown: func(e *echo.Echo) { + }), + server.WithShutdown(func(e *echo.Echo) { // Cleanup: close DB pools, flush telemetry, etc. sqlDB, _ := pool.DB() _ = sqlDB.Close() - }, + }), +) +if err != nil { + log.Fatal(err) } -server.StartWithConfig(serverCfg) +// Wire your own shutdown trigger (e.g. SIGTERM), then Start blocks until it fires. +go func() { + <-shutdownSignal + _ = srv.Shutdown(context.Background()) +}() + +if err := srv.Start(); err != nil { + log.Fatal(err) +} ``` ### Built-in Health Endpoints @@ -957,7 +980,7 @@ The `server` package automatically registers: There is no built-in `GET /` handler — register your own routes via `EchoConfigurer`. -**Note:** `server.Config` has an `OTelConfig` field (`yaml:"-" mapstructure:"-"`), used for OTel-based logging during startup/shutdown. Set it directly on the config or via `server.WithOTelConfig()`. HTTP request tracing is not added automatically — add tracing middleware through the `Middleware` slice or inside `EchoConfigurer`. +**Note:** `server.Config` has an `OTelConfig` field (`yaml:"-" mapstructure:"-"`). Set it directly on the config or via `server.WithOTelConfig()`. When set, the server auto-installs OTel request instrumentation (instrumentation scope `http.server`): tracing spans named `{method} {route}` with attributes `http.request.method`, `url.full`, `http.response.status_code`, and `http.route`; plus metrics `http.server.request.count` and `http.server.request.duration` attributed by method and status code. See `server/README.md` for details. --- @@ -971,7 +994,7 @@ Single port serves both gRPC and REST via HTTP/2 cleartext: ```go import ( - "github.com/jasoet/pkg/v2/grpc" + "github.com/jasoet/pkg/v3/grpc" "google.golang.org/grpc" pb "myapp/proto/gen" ) @@ -1085,7 +1108,7 @@ If your application needs async jobs, background processing, or scheduled tasks, `temporal.Config` has two serializable fields (`HostPort`, `Namespace`) plus the usual `OTelConfig *otel.Config` field tagged `yaml:"-" mapstructure:"-"` (injected at runtime, never serialized): ```go -import "github.com/jasoet/pkg/v2/temporal" +import "github.com/jasoet/pkg/v3/temporal" // In AppConfig: Temporal temporal.Config `yaml:"temporal" mapstructure:"temporal"` @@ -1176,8 +1199,8 @@ import ( "myapp/internal/service" "myapp/migrations" - "github.com/jasoet/pkg/v2/db" - "github.com/jasoet/pkg/v2/temporal" + "github.com/jasoet/pkg/v3/db" + "github.com/jasoet/pkg/v3/temporal" ) const taskQueue = "myapp-tasks" @@ -1192,11 +1215,15 @@ func main() { } // Database (activities need repos) - pool, err := cfg.Database.Pool() + pool, err := db.NewPool(db.WithConnectionConfig(cfg.Database)) if err != nil { log.Fatalf("failed to connect to database: %v", err) } - if err := db.RunPostgresMigrationsWithGorm(context.Background(), pool, migrations.FS, "."); err != nil { + sqlDB, err := pool.DB() + if err != nil { + log.Fatalf("failed to get sql.DB: %v", err) + } + if err := db.RunPostgresMigrations(context.Background(), sqlDB, migrations.FS, "."); err != nil { log.Fatalf("failed to run migrations: %v", err) } @@ -1205,12 +1232,19 @@ func main() { paymentSvc := service.NewPaymentService(orderRepo) activities := apptemporal.NewActivities(orderRepo, paymentSvc) - // Create WorkerManager (owns its own Temporal client) - wm, err := temporal.NewWorkerManager(&cfg.Temporal) + // Create the Temporal client (caller-owned) + temporalClient, err := temporal.NewClient(temporal.WithConfig(cfg.Temporal)) + if err != nil { + log.Fatalf("failed to create temporal client: %v", err) + } + defer temporalClient.Close() + + // Create WorkerManager (borrows the client; Close does not close it) + wm, err := temporal.NewWorkerManager(temporalClient) if err != nil { log.Fatalf("failed to create worker manager: %v", err) } - defer wm.Close() + defer wm.Close(context.Background()) // Register worker with workflows and activities w := wm.Register(taskQueue, worker.Options{}) @@ -1229,7 +1263,7 @@ func main() { ```go // In your handler or service: -temporalClient, err := temporal.NewClient(&cfg.Temporal) +temporalClient, err := temporal.NewClient(temporal.WithConfig(cfg.Temporal)) if err != nil { return err } @@ -1272,7 +1306,7 @@ sm.DeleteSchedule(ctx, "daily-cleanup") New in v2.13.0: the `temporal/job` package provides a `Definition` — a typed handle for one registered workflow, bundling registration, execution, scheduling, and lifecycle control: ```go -import "github.com/jasoet/pkg/v2/temporal/job" +import "github.com/jasoet/pkg/v3/temporal/job" def, err := job.New("orders-sync", "myapp-tasks", job.WithRegister(func(w worker.Worker) { @@ -1296,7 +1330,7 @@ For integration tests against a real Temporal server: ```go //go:build integration -import "github.com/jasoet/pkg/v2/temporal/testcontainer" +import "github.com/jasoet/pkg/v3/temporal/testcontainer" func TestWorkflow(t *testing.T) { ctx := context.Background() @@ -2024,16 +2058,16 @@ tasks: | Configuration | `config` | `config.LoadString[T](yaml, prefix...)` | | OpenTelemetry | `otel` | `otel.NewConfig(name)`, `otel.Layers.Start*()`, `otel.F(k, v)` | | OTel Logging | `otel` | `otel.NewLoggerProviderWithOptions(name, opts...)` | -| Legacy Logging | `logging` | `logging.Initialize(name, debug)` | -| Database Pool | `db` | `db.ConnectionConfig{...}.Pool()` | -| Migrations | `db` | `db.RunPostgresMigrationsWithGorm(ctx, pool, fs, path)` | -| HTTP Server | `server` | `server.StartWithConfig(cfg)`, `server.DefaultConfig(port, op, shut)` | +| Global Logger | `otel` | `otel.Initialize(name, debug)`, `otel.ContextLogger(ctx, component)` | +| Database Pool | `db` | `db.NewPool(db.WithConnectionConfig(cfg), db.WithOTelConfig(otelCfg))` | +| Migrations | `db` | `db.RunPostgresMigrations(ctx, sqlDB, fs, path)` (`sqlDB, _ := pool.DB()`) | +| HTTP Server | `server` | `server.New(opts...)`, `srv.Start()`, `srv.Shutdown(ctx)` | | gRPC Server | `grpc` | `grpc.New(opts...)`, `grpc.Start(port, registrar, opts...)` | | REST Client | `rest` | `rest.NewClient(opts...)`, `client.MakeRequestWithTrace(...)` | -| Retry | `retry` | `retry.Do(ctx, cfg, op)`, `retry.DefaultConfig().WithName(n).WithOTel(c)` | +| Retry | `retry` | `retry.Do(ctx, cfg, op)`, `retry.New(retry.WithName(n), retry.WithOTelConfig(c))` | | Concurrency | `concurrent` | `concurrent.ExecuteConcurrently(ctx, funcs)` | -| Temporal Client | `temporal` | `temporal.NewClient(cfg)` | -| Temporal Worker | `temporal` | `temporal.NewWorkerManager(cfg)`, `wm.Register(queue, opts)` | +| Temporal Client | `temporal` | `temporal.NewClient(temporal.WithConfig(cfg))` | +| Temporal Worker | `temporal` | `temporal.NewWorkerManager(client)`, `wm.Register(queue, opts)` | | Temporal Schedule | `temporal` | `temporal.NewScheduleManager(client)`, `sm.CreateWorkflowSchedule(...)` | | Temporal Test | `temporal/testcontainer` | `testcontainer.Setup(ctx, cfg, opts)` | | Docker | `docker` | `docker.New(opts...)`, `docker.NewFromRequest(req)` | @@ -2065,9 +2099,9 @@ import ( dashboardmod "myapp/internal/dashboard" "myapp/migrations" - "github.com/jasoet/pkg/v2/db" - "github.com/jasoet/pkg/v2/otel" - "github.com/jasoet/pkg/v2/server" + "github.com/jasoet/pkg/v3/db" + "github.com/jasoet/pkg/v3/otel" + "github.com/jasoet/pkg/v3/server" _ "myapp/docs" // swagger generated docs ) @@ -2095,14 +2129,20 @@ func main() { otelCfg := otel.NewConfig("myapp") // --- Database --- - cfg.Database.OTelConfig = otelCfg - pool, err := cfg.Database.Pool() + pool, err := db.NewPool( + db.WithConnectionConfig(cfg.Database), + db.WithOTelConfig(otelCfg), + ) if err != nil { log.Fatalf("failed to connect to database: %v", err) } // --- Migrations --- - if err := db.RunPostgresMigrationsWithGorm(context.Background(), pool, migrations.FS, "."); err != nil { + sqlDB, err := pool.DB() + if err != nil { + log.Fatalf("failed to get sql.DB: %v", err) + } + if err := db.RunPostgresMigrations(context.Background(), sqlDB, migrations.FS, "."); err != nil { log.Fatalf("failed to run migrations: %v", err) } @@ -2122,29 +2162,41 @@ func main() { dashboardHandler := dashboardmod.NewHandler(dashboardSvc) // --- Server --- - server.StartWithConfig(server.Config{ - Port: cfg.Server.Port, - ShutdownTimeout: cfg.Server.ShutdownTimeout, - Middleware: []echo.MiddlewareFunc{ + srv, err := server.New( + server.WithPort(cfg.Server.Port), + server.WithShutdownTimeout(cfg.Server.ShutdownTimeout), + server.WithMiddleware( middleware.Recover(), middleware.Logger(), - }, - EchoConfigurer: func(e *echo.Echo) { + ), + server.WithEchoConfigurer(func(e *echo.Echo) { e.GET("/swagger/*", echoSwagger.WrapHandler) apiV1 := e.Group("/api/v1") userHandler.RegisterRoutes(apiV1.Group("/users")) dashboardHandler.RegisterRoutes(apiV1.Group("/dashboard")) - }, - Operation: func(e *echo.Echo) {}, - Shutdown: func(e *echo.Echo) { + }), + server.WithOperation(func(e *echo.Echo) {}), + server.WithShutdown(func(e *echo.Echo) { log.Println("Shutting down...") if sqlDB, err := pool.DB(); err == nil { _ = sqlDB.Close() } _ = otelCfg.Shutdown(context.Background()) - }, - }) + }), + ) + if err != nil { + log.Fatal(err) + } + + go func() { + <-shutdownSignal // e.g. from signal.NotifyContext + _ = srv.Shutdown(context.Background()) + }() + + if err := srv.Start(); err != nil { + log.Fatal(err) + } } ``` diff --git a/README.md b/README.md index 17b3bc3..947e011 100644 --- a/README.md +++ b/README.md @@ -1,22 +1,22 @@ -# Go Utility Packages (v2) +# Go Utility Packages (v3) [![Go Version](https://img.shields.io/badge/Go-1.26+-blue.svg)](https://golang.org) [![License: MIT](https://img.shields.io/badge/License-MIT-yellow.svg)](https://opensource.org/licenses/MIT) [![Build Status](https://github.com/jasoet/pkg/actions/workflows/release.yml/badge.svg)](https://github.com/jasoet/pkg/actions) -[![Test Coverage](https://img.shields.io/badge/coverage-79%25-brightgreen.svg)](https://github.com/jasoet/pkg) -[![Go Report Card](https://goreportcard.com/badge/github.com/jasoet/pkg/v2)](https://goreportcard.com/report/github.com/jasoet/pkg/v2) +[![Go Report Card](https://goreportcard.com/badge/github.com/jasoet/pkg/v3)](https://goreportcard.com/report/github.com/jasoet/pkg/v3) Production-ready Go utility packages with **OpenTelemetry** instrumentation, comprehensive testing, and battle-tested components for building modern cloud-native applications. ## Versioning -**Current Release:** `v2.13.0` (GA) -**Status:** Production Ready -**Test Coverage:** 79% +**Current Release (v2, frozen):** `v2.13.1` — maintenance-only on [`release/v2`](https://github.com/jasoet/pkg/tree/release/v2). +**In Development (v3):** `github.com/jasoet/pkg/v3` on the `next` branch (prereleases `v3.0.0-next.N`). +**Status:** v3 in active development +**Test Coverage:** Reported per package below; regenerate with `task test:complete`. -> **v2 Highlights:** OpenTelemetry instrumentation across all packages, 79% test coverage, modernized dependencies +> **v3 Highlights:** OpenTelemetry instrumentation across all packages (the former `logging` package merged into `otel`), unified `WithOTelConfig` injection, functional options everywhere, third-party types de-leaked from public signatures, modernized dependencies. > -> **Breaking Change:** v1 does not include OpenTelemetry. v2 adds optional OTel support with minimal API changes. +> **v3 is a breaking release.** It is a single big-bang major rather than a series of them ([ADR 0001](./docs/adr/0001-freeze-v2-and-ship-v3-as-one-big-bang.md)). Read **[MIGRATION.md](./MIGRATION.md)** before upgrading — it covers every break, including several that do not appear in the generated release notes. Because the module path changed, `/v2` and `/v3` can be imported side by side while you migrate. ### v1 Availability @@ -26,7 +26,7 @@ The v1 release is preserved on the [`release/v1`](https://github.com/jasoet/pkg/ go get github.com/jasoet/pkg@v1.6.0 ``` -**Note:** v1 is no longer actively maintained. All new development targets v2. +**Note:** v1 is no longer actively maintained. All new development targets v3 (`github.com/jasoet/pkg/v3`). ## Packages @@ -36,7 +36,6 @@ Production-ready components with comprehensive observability, testing, and examp |---------|-------------|--------------| | **[otel](./otel/)** | OpenTelemetry integration | Tracing, metrics, logging, unified config | | **[config](./config/)** | YAML configuration with env overrides | Type-safe, generics, nested env vars | -| **[logging](./logging/)** | Structured logging with zerolog | Context-aware, OTel integration | | **[db](./db/)** | Multi-database support | PostgreSQL, MySQL, MSSQL, migrations, OTel | | **[docker](./docker/)** | Docker container executor | Lifecycle management, wait strategies, dual API | | **[argo](./argo/)** | Argo Workflows client | Kubernetes API, Argo Server, OTel, flexible config | @@ -55,7 +54,7 @@ Production-ready components with comprehensive observability, testing, and examp ### Installation ```bash -go get github.com/jasoet/pkg/v2@latest +go get github.com/jasoet/pkg/v3@latest ``` ### Basic Usage @@ -64,9 +63,9 @@ go get github.com/jasoet/pkg/v2@latest package main import ( - "github.com/jasoet/pkg/v2/config" - "github.com/jasoet/pkg/v2/logging" - "github.com/jasoet/pkg/v2/server" + "github.com/jasoet/pkg/v3/config" + "github.com/jasoet/pkg/v3/server" + "github.com/jasoet/pkg/v3/otel" "github.com/labstack/echo/v4" "github.com/rs/zerolog/log" ) @@ -77,7 +76,7 @@ type AppConfig struct { func main() { // Setup logging - if err := logging.Initialize("my-service", false); err != nil { + if err := otel.Initialize("my-service", false); err != nil { log.Fatal().Err(err).Msg("failed to initialize logging") } @@ -95,9 +94,21 @@ func main() { // cleanup } - // Start HTTP server - serverCfg := server.DefaultConfig(cfg.Port, operation, shutdown) - if err := server.StartWithConfig(serverCfg); err != nil { + // Start HTTP server (blocks until Shutdown is called) + srv, err := server.New( + server.WithPort(cfg.Port), + server.WithOperation(operation), + server.WithShutdown(shutdown), + ) + if err != nil { + log.Fatal().Err(err).Msg("invalid server config") + } + go func() { + // trigger shutdown however you like, e.g. on SIGTERM + <-signalChan + _ = srv.Shutdown(context.Background()) + }() + if err := srv.Start(); err != nil { log.Fatal().Err(err).Msg("server failed") } } @@ -109,7 +120,6 @@ Each package includes comprehensive examples: ```bash # Run specific package examples -go run -tags=example ./examples/logging go run -tags=example ./examples/db go run -tags=example ./examples/server @@ -117,14 +127,13 @@ go run -tags=example ./examples/server go build -tags=example ./... ``` -Examples for all packages live in the top-level `examples/` directory (e.g. `./examples/logging/`). +Examples for all packages live in the top-level `examples/` directory (e.g. `./examples/otel/`). ## Test Coverage -**Overall Coverage: 79%** (unit + integration suites; Argo tests require a k8s cluster and are not included) +Per-package figures are shown next to each package in the [Packages](#packages) section above. They come from the unit **and** integration suites — regenerate with `task test:integration` (needs Docker or Podman; writes `output/coverage-integration.html`). -### Package Coverage -- base32 (99%), config (98%), concurrent (95%), logging (95%), rest (93%), argo (91%), otel (85%), docker (83%), compress (82%), temporal (81%), retry (79%), ssh (78%), server (77%), db (77%), grpc (71%) +Argo tests are excluded from those figures because they need a live k8s cluster with Argo Workflows. To include them, run `task test:complete` (writes `output/coverage-complete.html`). ### Run Tests @@ -137,7 +146,7 @@ task test:integration # Complete test suite with coverage report task test:complete -open output/coverage-all.html +open output/coverage-complete.html ``` ## Key Features @@ -230,7 +239,7 @@ logger := config.GetLogger("my-service") ``` **Features:** Automatic instrumentation, context propagation, graceful shutdown -**Coverage:** 84.8% | **[Examples](./examples/otel/)** | **[Documentation](./otel/README.md)** +**Coverage:** 89.5% | **[Examples](./examples/otel/)** | **[Documentation](./otel/README.md)** #### [config](./config/) - Configuration Management Type-safe YAML configuration with environment variable overrides. @@ -247,28 +256,7 @@ cfg, _ := config.LoadString[AppConfig](yamlContent, "APP") ``` **Features:** Environment variable overrides, nested env vars, generics-based loading -**Coverage:** 97.6% | **[Examples](./examples/config/)** | **[Documentation](./config/README.md)** - -#### [logging](./logging/) - Structured Logging -Zerolog-based OTel LoggerProvider with automatic trace correlation. - -```go -// Create LoggerProvider -loggerProvider := logging.NewLoggerProvider("my-service", false) - -// Use with OTel config -otelCfg := &otel.Config{ - LoggerProvider: loggerProvider, - // ... other config -} - -// Or use legacy zerolog -_ = logging.Initialize("my-service", false) -log.Info().Str("user", "john").Msg("User logged in") -``` - -**Features:** Context-aware, OTel log provider, performance optimized -**Coverage:** 94.6% | **[Examples](./examples/logging/)** | **[Documentation](./logging/README.md)** +**Coverage:** 96.4% | **[Examples](./examples/config/)** | **[Documentation](./config/README.md)** ### Data Access @@ -276,22 +264,24 @@ log.Info().Str("user", "john").Msg("User logged in") PostgreSQL, MySQL, MSSQL support with GORM and migrations. ```go -pool, _ := db.ConnectionConfig{ - DBType: db.Postgresql, - Host: "localhost", - Port: 5432, - Username: "user", - Password: "pass", - DBName: "mydb", - OTelConfig: otelConfig, -}.Pool() +pool, _ := db.NewPool( + db.WithConnectionConfig(db.ConnectionConfig{ + DBType: db.Postgresql, + Host: "localhost", + Port: 5432, + Username: "user", + Password: "pass", + DBName: "mydb", + }), + db.WithOTelConfig(otelConfig), +) // Automatic query tracing and metrics pool.Find(&users) ``` **Features:** Connection pooling, migrations, OTel tracing, health monitoring -**Coverage:** 76.7% | **[Examples](./examples/db/)** | **[Documentation](./db/README.md)** +**Coverage:** 83.5% | **[Examples](./examples/db/)** | **[Documentation](./db/README.md)** #### [docker](./docker/) - Docker Container Executor Production-ready Docker container management with dual API styles. @@ -325,7 +315,7 @@ exec, _ := docker.NewFromRequest(req) ``` **Features:** Lifecycle management, wait strategies, log streaming, dual API (functional + struct) -**Coverage:** 83.1% | **[Examples](./examples/docker/)** | **[Documentation](./docker/README.md)** +**Coverage:** 84.1% | **[Examples](./examples/docker/)** | **[Documentation](./docker/README.md)** #### [argo](./argo/) - Argo Workflows Client Production-ready Argo Workflows client with flexible configuration. @@ -355,16 +345,17 @@ ctx, client, err := argo.NewClientWithOptions(ctx, ``` **Features:** Multiple connection modes, functional options, OTel support, proper error handling -**[Examples](./examples/argo/)** | **[Documentation](./argo/README.md)** +**Coverage:** 94.8% | **[Examples](./examples/argo/)** | **[Documentation](./argo/README.md)** #### [retry](./retry/) - Retry with Exponential Backoff Production-ready retry mechanism using `cenkalti/backoff/v4` with OTel instrumentation. ```go -cfg := retry.DefaultConfig(). - WithName("db.connect"). - WithMaxRetries(3). - WithOTel(otelConfig) +cfg := retry.New( + retry.WithName("db.connect"), + retry.WithMaxRetries(3), + retry.WithOTelConfig(otelConfig), +) err := retry.Do(ctx, cfg, func(ctx context.Context) error { return db.Ping(ctx) @@ -375,7 +366,7 @@ return retry.Permanent(fmt.Errorf("invalid config")) ``` **Features:** Exponential backoff, context-aware, OTel tracing, permanent error marking -**[Documentation](./retry/README.md)** +**Coverage:** 100.0% | **[Examples](./examples/retry/)** | **[Documentation](./retry/README.md)** #### [base32](./base32/) - Crockford Base32 Encoding Crockford Base32 encoding with CRC-10 checksums for human-readable, error-correcting identifiers. @@ -397,7 +388,7 @@ normalized := base32.NormalizeBase32("ab-CD iL o9") // "ABCD1109" ``` **Features:** URL-safe alphabet, automatic error correction, CRC-10 checksums, compact encoding -**[Examples](./examples/base32/)** | **[Documentation](./base32/README.md)** +**Coverage:** 100.0% | **[Examples](./examples/base32/)** | **[Documentation](./base32/README.md)** ### HTTP & gRPC @@ -413,14 +404,21 @@ shutdown := func(e *echo.Echo) { // cleanup } -config := server.DefaultConfig(8080, operation, shutdown) -if err := server.StartWithConfig(config); err != nil { +srv, err := server.New( + server.WithPort(8080), + server.WithOperation(operation), + server.WithShutdown(shutdown), +) +if err != nil { + log.Fatal().Err(err).Msg("invalid server config") +} +if err := srv.Start(); err != nil { // blocks until srv.Shutdown(ctx) log.Fatal().Err(err).Msg("server failed") } ``` **Features:** Health checks, graceful shutdown, middleware -**Coverage:** 77.1% | **[Examples](./examples/server/)** | **[Documentation](./server/README.md)** +**Coverage:** 97.0% | **[Examples](./examples/server/)** | **[Documentation](./server/README.md)** #### [grpc](./grpc/) - gRPC Server Production-ready gRPC with Echo gateway integration. @@ -439,7 +437,7 @@ server.Start() ``` **Features:** H2C mode, dual HTTP/gRPC, gateway, observability -**Coverage:** 71.2% | **[Examples](./examples/grpc/)** | **[Documentation](./grpc/README.md)** +**Coverage:** 82.0% | **[Examples](./examples/grpc/)** | **[Documentation](./grpc/README.md)** #### [rest](./rest/) - HTTP Client Resilient REST client with OTel tracing. @@ -461,7 +459,7 @@ response, _ := client.MakeRequestWithTrace(ctx, "GET", url, "", headers) ``` **Features:** Retries, tracing, middleware support -**Coverage:** 92.9% | **[Examples](./examples/rest/)** | **[Documentation](./rest/README.md)** +**Coverage:** 93.0% | **[Examples](./examples/rest/)** | **[Documentation](./rest/README.md)** ### Utilities @@ -482,7 +480,7 @@ results, _ := concurrent.ExecuteConcurrently(ctx, funcs) ``` **Features:** Go 1.26+ generics, error aggregation, context support -**Coverage:** 95.1% | **[Examples](./examples/concurrent/)** | **[Documentation](./concurrent/README.md)** +**Coverage:** 100.0% | **[Examples](./examples/concurrent/)** | **[Documentation](./concurrent/README.md)** #### [temporal](./temporal/) - Workflow Orchestration Temporal workflow integration with observability. @@ -505,7 +503,7 @@ handle, _ := manager.CreateWorkflowSchedule(ctx, "daily-job", temporal.WorkflowS ``` **Features:** Schedule management, workers, job definitions, monitoring -**Coverage:** 81.2% | **[Examples](./examples/temporal/)** | **[Documentation](./temporal/README.md)** +**Coverage:** 84.5% | **[Examples](./examples/temporal/)** | **[Documentation](./temporal/README.md)** #### [ssh](./ssh/) - SSH Tunneling Secure SSH tunneling and port forwarding. @@ -527,7 +525,7 @@ defer tunnel.Close() ``` **Features:** Port forwarding, connection pooling, error handling -**Coverage:** 78.2% | **[Examples](./examples/ssh/)** | **[Documentation](./ssh/README.md)** +**Coverage:** 85.6% | **[Examples](./examples/ssh/)** | **[Documentation](./ssh/README.md)** #### [compress](./compress/) - File Compression Secure file compression with validation. @@ -544,7 +542,7 @@ compress.TarGz("/path/to/directory", outputFile) ``` **Features:** gzip, tar.gz, security validation, path traversal protection -**Coverage:** 82.4% | **[Examples](./examples/compress/)** | **[Documentation](./compress/README.md)** +**Coverage:** 85.3% | **[Examples](./examples/compress/)** | **[Documentation](./compress/README.md)** ## Contributing diff --git a/Taskfile.yml b/Taskfile.yml index 0090c7a..17d33ec 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -48,7 +48,7 @@ tasks: silent: true cmds: - mkdir -p output - - '{{.N}} go test -race -count=1 -coverprofile=output/coverage.out -covermode=atomic ./... -tags=!examples' + - '{{.N}} go test -race -count=1 -coverprofile=output/coverage.out -covermode=atomic ./...' - '{{.N}} go tool cover -html=output/coverage.out -o output/coverage.html' - 'echo "✓ Coverage: output/coverage.html"' @@ -233,7 +233,7 @@ tasks: desc: Run unit tests for CI (no coverage HTML) silent: true cmds: - - '{{.N}} go test -race -count=1 ./... -tags=!examples' + - '{{.N}} go test -race -count=1 ./...' ci:lint: desc: Run golangci-lint for CI @@ -261,8 +261,8 @@ tasks: - | LATEST_TAG=$(git describe --tags --abbrev=0 2>/dev/null || echo "") if [ -n "$LATEST_TAG" ]; then - echo "Warming Go proxy for github.com/jasoet/pkg/v2@${LATEST_TAG}" - {{.N}} bash -c "GOPROXY=https://proxy.golang.org GO111MODULE=on go list -m \"github.com/jasoet/pkg/v2@${LATEST_TAG}\"" || true + echo "Warming Go proxy for github.com/jasoet/pkg/v3@${LATEST_TAG}" + {{.N}} bash -c "GOPROXY=https://proxy.golang.org GO111MODULE=on go list -m \"github.com/jasoet/pkg/v3@${LATEST_TAG}\"" || true fi clean: diff --git a/argo/README.md b/argo/README.md index 740d0dd..c9c5130 100644 --- a/argo/README.md +++ b/argo/README.md @@ -1,23 +1,26 @@ # Argo Workflows Client -[![Go Version](https://img.shields.io/badge/Go-1.25+-blue.svg)](https://golang.org) +[![Go Version](https://img.shields.io/badge/Go-1.26+-blue.svg)](https://golang.org) [![License: MIT](https://img.shields.io/badge/License-MIT-yellow.svg)](https://opensource.org/licenses/MIT) -Production-ready Argo Workflows client library with flexible configuration, OpenTelemetry support, and comprehensive error handling. +Argo Workflows client library with flexible configuration, context-based OpenTelemetry propagation, and proper error handling. + +## Package Posture + +This package is an **SDK integration**: it exposes `argo-workflows` types (`apiclient.Client`, `v1alpha1.Workflow`, `workflow.*Request`) directly by design, rather than wrapping them behind an abstraction layer. It adds value on top of the raw SDK: unified client construction (kubeconfig / in-cluster / Argo Server), a fluent workflow builder, pre-built patterns, and optional OpenTelemetry instrumentation. If you need an SDK operation that is not wrapped here, use `client.NewWorkflowServiceClient()` directly. ## Features - **Multiple Connection Modes**: Kubernetes API, In-Cluster, or Argo Server HTTP - **Flexible Configuration**: Config structs and functional options -- **OpenTelemetry Integration**: Built-in tracing and observability +- **Context-Based OpenTelemetry**: OTel config propagates through `context.Context` — set it once at client creation, operations pick it up automatically - **Production-Ready**: Proper error handling, no fatal errors -- **Type-Safe**: Full Go type safety with generics support - **Well-Documented**: Comprehensive examples and documentation ## Installation ```bash -go get github.com/jasoet/pkg/v2/argo +go get github.com/jasoet/pkg/v3/argo ``` ## Quick Start @@ -29,7 +32,8 @@ package main import ( "context" - "github.com/jasoet/pkg/v2/argo" + + "github.com/jasoet/pkg/v3/argo" ) func main() { @@ -94,6 +98,11 @@ ctx, client, err := argo.NewClientWithOptions(ctx, Connect via Argo Server HTTP API. ```go +ctx, client, err := argo.NewClient(ctx, + argo.ServerConfig("https://argo-server:2746", "Bearer token"), +) + +// Or using functional options ctx, client, err := argo.NewClientWithOptions(ctx, argo.WithArgoServer("https://argo-server:2746", "Bearer token"), ) @@ -112,19 +121,19 @@ ctx, client, err := argo.NewClientWithOptions(ctx, ```go type Config struct { // KubeConfigPath specifies the path to kubeconfig file - KubeConfigPath string + KubeConfigPath string `yaml:"kubeConfigPath" mapstructure:"kubeConfigPath"` // Context specifies the kubeconfig context to use - Context string + Context string `yaml:"context" mapstructure:"context"` // InCluster indicates whether to use in-cluster configuration - InCluster bool + InCluster bool `yaml:"inCluster" mapstructure:"inCluster"` // ArgoServerOpts configures connection to Argo Server - ArgoServerOpts ArgoServerOpts + ArgoServerOpts ServerOpts `yaml:"argoServer" mapstructure:"argoServer"` // OTelConfig enables OpenTelemetry instrumentation - OTelConfig *otel.Config + OTelConfig *otel.Config `yaml:"-" mapstructure:"-"` } ``` @@ -138,7 +147,7 @@ config := argo.DefaultConfig() config := argo.InClusterConfig() // Argo Server configuration -config := argo.ArgoServerConfig("https://argo-server:2746", "Bearer token") +config := argo.ServerConfig("https://argo-server:2746", "Bearer token") ``` ## Functional Options @@ -156,6 +165,13 @@ ctx, client, err := argo.NewClientWithOptions(ctx, argo.WithArgoServer("https://argo-server:2746", "Bearer token"), argo.WithArgoServerInsecure(false), argo.WithArgoServerHTTP1(false), + argo.WithArgoServerOpts(argo.ServerOpts{ + URL: "https://argo-server:2746", + AuthToken: "Bearer token", + }), + + // Apply a complete pre-built config + argo.WithConfig(myConfig), // Observability argo.WithOTelConfig(otelConfig), @@ -164,28 +180,43 @@ ctx, client, err := argo.NewClientWithOptions(ctx, ## OpenTelemetry Integration -Enable distributed tracing and monitoring: +OTel configuration propagates through `context.Context`. `NewClient` / `NewClientWithOptions` inject the configured `*otel.Config` into the context they return; the package operations (`SubmitWorkflow`, `SubmitAndWait`, `GetWorkflowStatus`, `ListWorkflows`, `DeleteWorkflow`) resolve it from the context via `otel.ConfigFromContext(ctx)`. Pass the returned `ctx` to operations and instrumentation is automatic — no per-call config argument. ```go import ( - "github.com/jasoet/pkg/v2/argo" - "github.com/jasoet/pkg/v2/otel" + "github.com/jasoet/pkg/v3/argo" + "github.com/jasoet/pkg/v3/otel" ) // Create OTel config -otelConfig := otel.NewConfig("my-service"). - WithTracerProvider(tracerProvider). - WithMeterProvider(meterProvider) +otelConfig := otel.NewConfig("my-service", + otel.WithTracerProvider(tracerProvider), + otel.WithMeterProvider(meterProvider), +) -// Create Argo client with OTel +// Create Argo client with OTel — the returned ctx carries the config ctx, client, err := argo.NewClientWithOptions(ctx, argo.WithKubeConfig("/path/to/kubeconfig"), argo.WithOTelConfig(otelConfig), ) +if err != nil { + return err +} + +// Operations read the OTel config from ctx +created, err := argo.SubmitWorkflow(ctx, client, wf) +``` + +You can also inject a config into any context manually: + +```go +ctx = otel.ContextWithConfig(ctx, otelConfig) ``` ## Working with Workflows +The examples below use the raw Argo SDK client (`client.NewWorkflowServiceClient()`). For the higher-level instrumented wrappers, see [Enhanced Client Operations](#enhanced-client-operations). + ### List Workflows ```go @@ -286,14 +317,15 @@ for { ## Workflow Builder API -The workflow builder API provides a high-level, fluent interface for constructing Argo Workflows without needing to understand the low-level protobuf-generated structs. It includes template sources, pre-built patterns, and full OpenTelemetry instrumentation. +The workflow builder API provides a high-level, fluent interface for constructing Argo Workflows without needing to understand the low-level protobuf-generated structs. It includes template sources, pre-built patterns, and optional OpenTelemetry instrumentation. ### Quick Start with Builder ```go import ( - "github.com/jasoet/pkg/v2/argo/builder" - "github.com/jasoet/pkg/v2/argo/builder/template" + "github.com/jasoet/pkg/v3/argo" + "github.com/jasoet/pkg/v3/argo/builder" + "github.com/jasoet/pkg/v3/argo/builder/template" ) // Create workflow steps @@ -317,8 +349,8 @@ if err != nil { return err } -// Submit workflow -created, err := argo.SubmitWorkflow(ctx, client, wf, otelConfig) +// Submit workflow (ctx carries the OTel config if one was set) +created, err := argo.SubmitWorkflow(ctx, client, wf) ``` ### Template Sources @@ -390,6 +422,10 @@ webhook := template.NewHTTP("notify", Configure workflows with functional options: ```go +// Values referenced by pointer below must be addressable, so declare them first. +retryLimit := intstr.FromInt32(3) // k8s.io/apimachinery/pkg/util/intstr +ttl := int32(3600) + wf, err := builder.NewWorkflowBuilder("myworkflow", "argo", // Service Account builder.WithServiceAccount("argo-workflow"), @@ -405,11 +441,15 @@ wf, err := builder.NewWorkflowBuilder("myworkflow", "argo", // Resource Management builder.WithArchiveLogs(true), - builder.WithActiveDeadline(3600), // 1 hour timeout + builder.WithActiveDeadlineSeconds(3600), // 1 hour timeout // Retry Strategy + // RetryStrategy.Limit is *intstr.IntOrString, so take the address of an intstr value + // (declared before the builder call: `retryLimit := intstr.FromInt32(3)`). + // The default retry strategy is applied to leaf templates only (never to the generated + // "main" or "exit-handler" step templates). builder.WithRetryStrategy(&v1alpha1.RetryStrategy{ - Limit: intstr.FromInt(3), + Limit: &retryLimit, RetryPolicy: "Always", }), @@ -421,6 +461,10 @@ wf, err := builder.NewWorkflowBuilder("myworkflow", "argo", }, }), + // Garbage collection and TTL + builder.WithPodGC(&v1alpha1.PodGC{Strategy: v1alpha1.PodGCOnWorkflowSuccess}), + builder.WithTTL(&v1alpha1.TTLStrategy{SecondsAfterCompletion: &ttl}), + // OpenTelemetry builder.WithOTelConfig(otelConfig), ).Build() @@ -460,7 +504,7 @@ wf, err := builder.NewWorkflowBuilder("deployment", "argo"). ##### Build-Test-Deploy ```go -import "github.com/jasoet/pkg/v2/argo/patterns" +import "github.com/jasoet/pkg/v3/argo/patterns" wf, err := patterns.BuildTestDeploy( "myapp", "argo", @@ -541,7 +585,7 @@ wf, err := patterns.MapReduce( "word-count", "argo", "alpine:latest", []string{"file1.txt", "file2.txt", "file3.txt"}, - "wc -w", // map command + "wc -w", // map command "awk '{sum+=$1} END {print sum}'", // reduce command ) ``` @@ -576,12 +620,12 @@ wf, err := patterns.ParallelDeployment( ### Enhanced Client Operations -Higher-level operations with full OpenTelemetry instrumentation: +Higher-level operations with optional OpenTelemetry instrumentation. None of them take an OTel config argument — they resolve it from `ctx` via `otel.ConfigFromContext(ctx)`. When the `ctx` came from `NewClient` / `NewClientWithOptions` configured with `WithOTelConfig`, instrumentation is automatic. #### Submit Workflow ```go -import "github.com/jasoet/pkg/v2/argo" +import "github.com/jasoet/pkg/v3/argo" wf, err := builder.NewWorkflowBuilder("deploy", "argo"). Add(deployStep). @@ -590,7 +634,7 @@ if err != nil { return err } -created, err := argo.SubmitWorkflow(ctx, client, wf, otelConfig) +created, err := argo.SubmitWorkflow(ctx, client, wf) if err != nil { return err } @@ -600,11 +644,24 @@ fmt.Printf("Workflow %s submitted\n", created.Name) #### Submit and Wait -Submit a workflow and wait for completion with automatic polling: +Submit a workflow and wait for completion with automatic polling. The poll interval +defaults to 5s and can be overridden with `argo.WithPollInterval`. The status is polled +immediately (so an already-terminal workflow is detected without waiting a full interval), +and permanent errors from the status API (e.g. `NotFound`, `PermissionDenied`) abort the +wait instead of spinning until the deadline. ```go -completed, err := argo.SubmitAndWait(ctx, client, wf, otelConfig, 10*time.Minute) +completed, err := argo.SubmitAndWait(ctx, client, wf, 10*time.Minute, + argo.WithPollInterval(15*time.Second)) // optional; default is 5s if err != nil { + switch { + case errors.Is(err, argo.ErrWaitTimeout): + // the wait deadline elapsed (also wraps context.DeadlineExceeded) + case errors.Is(err, argo.ErrWorkflowFailed): + // the workflow reached a terminal Failed/Error phase + case errors.Is(err, context.Canceled): + // the parent context was cancelled (distinct from a timeout) + } return err } @@ -616,7 +673,7 @@ if completed.Status.Phase == v1alpha1.WorkflowSucceeded { #### Get Workflow Status ```go -status, err := argo.GetWorkflowStatus(ctx, client, "argo", "my-workflow-abc123", otelConfig) +status, err := argo.GetWorkflowStatus(ctx, client, "argo", "my-workflow-abc123") if err != nil { return err } @@ -627,18 +684,21 @@ fmt.Printf("Progress: %s\n", status.Progress) #### List Workflows +`ListWorkflows` follows pagination continue tokens internally, so it returns the full +result set rather than a single (potentially truncated) page. + ```go // List all workflows -workflows, err := argo.ListWorkflows(ctx, client, "argo", "", otelConfig) +workflows, err := argo.ListWorkflows(ctx, client, "argo", "") // List with label selector -workflows, err := argo.ListWorkflows(ctx, client, "argo", "app=myapp", otelConfig) +workflows, err := argo.ListWorkflows(ctx, client, "argo", "app=myapp") ``` #### Delete Workflow ```go -err := argo.DeleteWorkflow(ctx, client, "argo", "my-workflow-abc123", otelConfig) +err := argo.DeleteWorkflow(ctx, client, "argo", "my-workflow-abc123") if err != nil { return err } @@ -675,12 +735,13 @@ package main import ( "context" + "fmt" "time" - "github.com/jasoet/pkg/v2/argo" - "github.com/jasoet/pkg/v2/argo/builder" - "github.com/jasoet/pkg/v2/argo/builder/template" - "github.com/jasoet/pkg/v2/otel" + "github.com/jasoet/pkg/v3/argo" + "github.com/jasoet/pkg/v3/argo/builder" + "github.com/jasoet/pkg/v3/argo/builder/template" + "github.com/jasoet/pkg/v3/otel" ) func main() { @@ -689,7 +750,7 @@ func main() { // Create OTel config otelConfig := otel.NewConfig("workflow-manager") - // Create Argo client + // Create Argo client — returned ctx carries the OTel config ctx, client, err := argo.NewClientWithOptions(ctx, argo.WithOTelConfig(otelConfig)) if err != nil { @@ -731,8 +792,8 @@ func main() { panic(err) } - // Submit and wait - completed, err := argo.SubmitAndWait(ctx, client, wf, otelConfig, 10*time.Minute) + // Submit and wait — OTel config resolved from ctx + completed, err := argo.SubmitAndWait(ctx, client, wf, 10*time.Minute) if err != nil { panic(err) } @@ -810,61 +871,50 @@ ctx, client, err := argo.NewClientWithOptions(ctx, ## Running Examples +Runnable examples live under `examples/argo/`, one directory per topic: + ```bash -# Run the comprehensive example -go run -tags=example ./examples/argo +# Run the basic client example +go run -tags=example ./examples/argo/basic # Or build and run -go build -tags=example -o argo-example ./examples/argo +go build -tags=example -o argo-example ./examples/argo/basic ./argo-example ``` See [examples/argo/README.md](../examples/argo/README.md) for more details. -## Comparison with Original Implementation +## Migration Notes + +### Migrating from v2 (positional OTel config) -### Before (scp/api) +The five package operations no longer take a positional `*otel.Config` argument. They resolve instrumentation from the context: ```go -// util/argo/argo.go - tightly coupled, uses fatal errors -func NewClient(ctx context.Context) (context.Context, apiclient.Client) { - ctx, argoClient, err := apiclient.NewClientFromOpts( - apiclient.Opts{ - ArgoServerOpts: apiclient.ArgoServerOpts{}, - ClientConfigSupplier: kube.GetCmdConfig, - Context: ctx, - }) - if err != nil { - log.Fatal().Err(err).Msg("unable to create argo client") // Fatal! - } - return ctx, argoClient -} +// Before (v2) +created, err := argo.SubmitWorkflow(ctx, client, wf, otelConfig) +completed, err := argo.SubmitAndWait(ctx, client, wf, otelConfig, 10*time.Minute) +status, err := argo.GetWorkflowStatus(ctx, client, "argo", name, otelConfig) +workflows, err := argo.ListWorkflows(ctx, client, "argo", "", otelConfig) +err = argo.DeleteWorkflow(ctx, client, "argo", name, otelConfig) + +// After (v3) — set the config once when creating the client +ctx, client, err := argo.NewClientWithOptions(ctx, argo.WithOTelConfig(otelConfig)) + +created, err := argo.SubmitWorkflow(ctx, client, wf) +completed, err := argo.SubmitAndWait(ctx, client, wf, 10*time.Minute) +status, err := argo.GetWorkflowStatus(ctx, client, "argo", name) +workflows, err := argo.ListWorkflows(ctx, client, "argo", "") +err = argo.DeleteWorkflow(ctx, client, "argo", name) ``` -### After (pkg/v2/argo) +If you construct clients without `WithOTelConfig` but still want instrumented calls, inject the config manually: ```go -// Flexible, reusable, proper error handling -ctx, client, err := argo.NewClientWithOptions(ctx, - argo.WithKubeConfig("/path/to/kubeconfig"), - argo.WithContext("production"), - argo.WithOTelConfig(otelConfig), -) -if err != nil { - return fmt.Errorf("failed to create client: %w", err) // Graceful! -} -defer client.Close() +ctx = otel.ContextWithConfig(ctx, otelConfig) ``` -## Benefits - -✅ **Reusable** - Can be used across multiple projects -✅ **Flexible** - Config struct + functional options -✅ **Library-friendly** - Returns errors instead of fatal -✅ **Testable** - Easy to mock and test -✅ **Observable** - OpenTelemetry integration ready -✅ **Well-documented** - Comprehensive docs and examples -✅ **Production-ready** - Proper error handling and logging +Also note: `argo.Option` no longer returns an error (it is now `func(*Config)`). The Argo Server config factory remains `argo.ServerConfig(...)` and the builder timeout option remains `builder.WithActiveDeadlineSeconds(seconds int64)` (unchanged from v2). Finally, instrument operations by threading the ctx returned by `NewClient`/`NewClientWithOptions` — calling operations with a fresh `context.Background()` compiles fine but silently disables instrumentation. ## Best Practices @@ -909,7 +959,8 @@ defer client.Close() go test ./argo # Integration tests (requires Kubernetes cluster) -go test -tags=integration ./argo +# Integration tests are gated behind the `argo` build tag (//go:build argo). +go test -tags=argo ./argo/... # All tests with coverage go test -cover ./argo diff --git a/argo/builder/builder.go b/argo/builder/builder.go index f0a633c..32ebc1f 100644 --- a/argo/builder/builder.go +++ b/argo/builder/builder.go @@ -2,7 +2,9 @@ package builder import ( "context" + "errors" "fmt" + "reflect" "strings" "time" @@ -12,7 +14,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "github.com/jasoet/pkg/v2/otel" + "github.com/jasoet/pkg/v3/otel" ) // WorkflowBuilder provides a fluent API for constructing Argo Workflows. @@ -55,18 +57,70 @@ type WorkflowBuilder struct { activeDeadlineSeconds *int64 // Workflow structure - entryPoint []v1alpha1.ParallelSteps - templates []v1alpha1.Template - exitHandlers []v1alpha1.ParallelSteps - metrics *v1alpha1.Metrics - uniqueTemplates map[string]struct{} - errors []error + entryPoint []v1alpha1.ParallelSteps + templates []v1alpha1.Template + exitHandlers []v1alpha1.ParallelSteps + // exitHandlersPriority holds cleanup/destroy steps that must run before other exit + // handlers. Kept separate so insertion order is preserved within each group instead of + // being reversed by repeated prepending. + exitHandlersPriority []v1alpha1.ParallelSteps + metrics *v1alpha1.Metrics + uniqueTemplates map[string]struct{} + errors []error + + // baseCtx is the parent context used to root builder trace spans. It defaults to + // context.Background() and can be overridden with WithContext so spans are children of + // the caller's trace instead of orphan roots. + baseCtx context.Context // OpenTelemetry otelConfig *otel.Config otel *otelInstrumentation } +// builderLogger wraps otel.LogHelper so that low-severity (Debug/Info) messages are only +// emitted when an OTel config is present. Without OTel, LogHelper falls back to an +// unleveled zerolog writer on stderr; suppressing Debug/Info there keeps the library quiet +// by default while still surfacing Warn/Error. +type builderLogger struct { + h *otel.LogHelper + verbose bool +} + +func (l *builderLogger) Debug(msg string, fields ...otel.Field) { + if l.verbose { + l.h.Debug(msg, fields...) + } +} + +func (l *builderLogger) Info(msg string, fields ...otel.Field) { + if l.verbose { + l.h.Info(msg, fields...) + } +} + +func (l *builderLogger) Warn(msg string, fields ...otel.Field) { l.h.Warn(msg, fields...) } + +func (l *builderLogger) Error(err error, msg string, fields ...otel.Field) { + l.h.Error(err, msg, fields...) +} + +// newLogger builds a leveled logger for the given function scope. +func (b *WorkflowBuilder) newLogger(ctx context.Context, function string) *builderLogger { + return &builderLogger{ + h: otel.NewLogHelper(ctx, b.otelConfig, "github.com/jasoet/pkg/v3/argo/builder", function), + verbose: b.otelConfig != nil, + } +} + +// context returns the builder's base context, defaulting to context.Background(). +func (b *WorkflowBuilder) context() context.Context { + if b.baseCtx != nil { + return b.baseCtx + } + return context.Background() +} + // NewWorkflowBuilder creates a new workflow builder with the specified name and namespace. // Additional configuration can be provided through functional options. // @@ -89,6 +143,7 @@ func NewWorkflowBuilder(name, namespace string, opts ...Option) *WorkflowBuilder uniqueTemplates: make(map[string]struct{}), labels: make(map[string]string), annotations: make(map[string]string), + baseCtx: context.Background(), } // Apply options @@ -113,7 +168,7 @@ func NewWorkflowBuilder(name, namespace string, opts ...Option) *WorkflowBuilder // deploy := template.NewContainer("deploy", "myapp:v1") // builder.Add(deploy) func (b *WorkflowBuilder) Add(source WorkflowSource) *WorkflowBuilder { - ctx := context.Background() + ctx := b.context() // Start tracing if b.otel != nil { @@ -122,14 +177,13 @@ func (b *WorkflowBuilder) Add(source WorkflowSource) *WorkflowBuilder { defer span.End() } - logger := otel.NewLogHelper(ctx, b.otelConfig, - "github.com/jasoet/pkg/v2/argo/builder", "WorkflowBuilder.Add") + logger := b.newLogger(ctx, "WorkflowBuilder.Add") logger.Debug("Adding workflow source") // Get templates from source templates, err := source.Templates() if err != nil { - b.errors = append(b.errors, fmt.Errorf("failed to get templates: %w", err)) + b.errors = append(b.errors, fmt.Errorf("%w: failed to get templates: %w", ErrTemplateSource, err)) logger.Error(err, "Failed to get templates from source") return b } @@ -142,7 +196,7 @@ func (b *WorkflowBuilder) Add(source WorkflowSource) *WorkflowBuilder { // Get steps from source steps, err := source.Steps() if err != nil { - b.errors = append(b.errors, fmt.Errorf("failed to get steps: %w", err)) + b.errors = append(b.errors, fmt.Errorf("%w: failed to get steps: %w", ErrTemplateSource, err)) logger.Error(err, "Failed to get steps from source") return b } @@ -175,7 +229,7 @@ func (b *WorkflowBuilder) Add(source WorkflowSource) *WorkflowBuilder { // parallelSource := &MyParallelSource{} // builder.AddParallel(parallelSource) func (b *WorkflowBuilder) AddParallel(source WorkflowSourceV2) *WorkflowBuilder { - ctx := context.Background() + ctx := b.context() // Start tracing if b.otel != nil { @@ -184,14 +238,13 @@ func (b *WorkflowBuilder) AddParallel(source WorkflowSourceV2) *WorkflowBuilder defer span.End() } - logger := otel.NewLogHelper(ctx, b.otelConfig, - "github.com/jasoet/pkg/v2/argo/builder", "WorkflowBuilder.AddParallel") + logger := b.newLogger(ctx, "WorkflowBuilder.AddParallel") logger.Debug("Adding parallel workflow source") // Get templates from source templates, err := source.Templates() if err != nil { - b.errors = append(b.errors, fmt.Errorf("failed to get templates: %w", err)) + b.errors = append(b.errors, fmt.Errorf("%w: failed to get templates: %w", ErrTemplateSource, err)) logger.Error(err, "Failed to get templates from source") return b } @@ -204,7 +257,7 @@ func (b *WorkflowBuilder) AddParallel(source WorkflowSourceV2) *WorkflowBuilder // Get parallel steps from source parallelSteps, err := source.ParallelSteps() if err != nil { - b.errors = append(b.errors, fmt.Errorf("failed to get parallel steps: %w", err)) + b.errors = append(b.errors, fmt.Errorf("%w: failed to get parallel steps: %w", ErrTemplateSource, err)) logger.Error(err, "Failed to get parallel steps from source") return b } @@ -239,10 +292,11 @@ func (b *WorkflowBuilder) AddParallel(source WorkflowSourceV2) *WorkflowBuilder // after the main workflow completes (regardless of success or failure). // // Note: Steps with names containing "destroy" or "cleanup" are automatically -// prioritized (prepended) in the exit handler sequence, ensuring resource -// cleanup runs before other exit steps. +// prioritized (run first) in the exit handler sequence, ensuring resource +// cleanup runs before other exit steps. Insertion order is preserved within the +// priority group and within the normal group. func (b *WorkflowBuilder) AddExitHandler(source WorkflowSource) *WorkflowBuilder { - ctx := context.Background() + ctx := b.context() // Start tracing if b.otel != nil { @@ -251,14 +305,13 @@ func (b *WorkflowBuilder) AddExitHandler(source WorkflowSource) *WorkflowBuilder defer span.End() } - logger := otel.NewLogHelper(ctx, b.otelConfig, - "github.com/jasoet/pkg/v2/argo/builder", "WorkflowBuilder.AddExitHandler") + logger := b.newLogger(ctx, "WorkflowBuilder.AddExitHandler") logger.Debug("Adding exit handler") // Get templates from source templates, err := source.Templates() if err != nil { - b.errors = append(b.errors, fmt.Errorf("failed to get exit handler templates: %w", err)) + b.errors = append(b.errors, fmt.Errorf("%w: failed to get exit handler templates: %w", ErrTemplateSource, err)) logger.Error(err, "Failed to get templates from exit handler") return b } @@ -271,24 +324,19 @@ func (b *WorkflowBuilder) AddExitHandler(source WorkflowSource) *WorkflowBuilder // Get steps from source steps, err := source.Steps() if err != nil { - b.errors = append(b.errors, fmt.Errorf("failed to get exit handler steps: %w", err)) + b.errors = append(b.errors, fmt.Errorf("%w: failed to get exit handler steps: %w", ErrTemplateSource, err)) logger.Error(err, "Failed to get steps from exit handler") return b } - // Add exit handler steps + // Add exit handler steps, appending to the priority or normal group. Appending (rather + // than prepending) preserves the relative insertion order within each group. for _, step := range steps { - // Check if this is a cleanup/destroy step and prioritize it - if strings.Contains(step.Name, "destroy") || strings.Contains(step.Name, "cleanup") { - // Insert at the beginning - b.exitHandlers = append([]v1alpha1.ParallelSteps{ - {Steps: []v1alpha1.WorkflowStep{step}}, - }, b.exitHandlers...) + ps := v1alpha1.ParallelSteps{Steps: []v1alpha1.WorkflowStep{step}} + if isPriorityExitStep(step.Name) { + b.exitHandlersPriority = append(b.exitHandlersPriority, ps) } else { - // Append normally - b.exitHandlers = append(b.exitHandlers, v1alpha1.ParallelSteps{ - Steps: []v1alpha1.WorkflowStep{step}, - }) + b.exitHandlers = append(b.exitHandlers, ps) } } @@ -336,7 +384,7 @@ func (b *WorkflowBuilder) WithMetrics(provider WorkflowMetricsProvider) *Workflo // log.Fatal(err) // } func (b *WorkflowBuilder) Build() (*v1alpha1.Workflow, error) { - ctx := context.Background() + ctx := b.context() // Start tracing and timing startTime := time.Now() @@ -352,81 +400,54 @@ func (b *WorkflowBuilder) Build() (*v1alpha1.Workflow, error) { }() } - logger := otel.NewLogHelper(ctx, b.otelConfig, - "github.com/jasoet/pkg/v2/argo/builder", "WorkflowBuilder.Build") + logger := b.newLogger(ctx, "WorkflowBuilder.Build") logger.Debug("Building workflow", otel.F("name", b.namePrefix), otel.F("namespace", b.namespace), otel.F("steps_count", len(b.entryPoint)), otel.F("templates_count", len(b.templates)), - otel.F("exit_handlers_count", len(b.exitHandlers))) + otel.F("exit_handlers_count", len(b.exitHandlers)+len(b.exitHandlersPriority))) // Check for errors - if len(b.errors) > 0 { + if err := b.joinedError(); err != nil { if b.otel != nil { - b.otel.recordError(ctx, "build_validation_error", b.errors[0]) + b.otel.recordError(ctx, "build_validation_error", err) } - logger.Error(b.errors[0], "Failed to build workflow") - return nil, b.errors[0] - } - - // Ensure we have at least one step - if len(b.entryPoint) == 0 { - logger.Warn("No steps provided, workflow will be empty") + logger.Error(err, "Failed to build workflow") + return nil, err } - // Build a fresh templates slice so Build() is safe to call multiple times. + // Build the entrypoint steps. If no steps were provided, insert a no-op step so the + // generated workflow is valid — Argo rejects a Steps template with zero steps. const entrypointName = "main" + entrySteps := b.entryPoint + if len(entrySteps) == 0 { + logger.Debug("No steps provided, inserting a no-op step") + entrySteps = []v1alpha1.ParallelSteps{{Steps: []v1alpha1.WorkflowStep{b.noopStep()}}} + } entrypoint := v1alpha1.Template{ Name: entrypointName, - Steps: b.entryPoint, + Steps: entrySteps, } + + // Build a fresh templates slice so Build() is safe to call multiple times. templates := make([]v1alpha1.Template, len(b.templates), len(b.templates)+2) copy(templates, b.templates) templates = append(templates, entrypoint) - // Create exit handler template if needed + // Create exit handler template if needed. const exitHandlerName = "exit-handler" var onExit string - if len(b.exitHandlers) > 0 { - exitHandler := v1alpha1.Template{ + exitSteps := b.orderedExitHandlers() + if len(exitSteps) > 0 { + templates = append(templates, v1alpha1.Template{ Name: exitHandlerName, - Steps: b.exitHandlers, - } - templates = append(templates, exitHandler) + Steps: exitSteps, + }) onExit = exitHandlerName } - // Build workflow - wf := &v1alpha1.Workflow{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: b.namePrefix, - Namespace: b.namespace, - Labels: b.labels, - Annotations: b.annotations, - }, - Spec: v1alpha1.WorkflowSpec{ - Entrypoint: entrypointName, - ServiceAccountName: b.serviceAccount, - Templates: templates, - Volumes: b.volumes, - Metrics: b.metrics, - ArchiveLogs: b.archiveLogs, - PodGC: b.podGC, - TTLStrategy: b.ttl, - ActiveDeadlineSeconds: b.activeDeadlineSeconds, - OnExit: onExit, - }, - } - - // Apply default retry strategy if set - if b.retryStrategy != nil { - for i := range wf.Spec.Templates { - if wf.Spec.Templates[i].RetryStrategy == nil { - wf.Spec.Templates[i].RetryStrategy = b.retryStrategy - } - } - } + wf := b.assembleWorkflow(entrypointName, onExit, templates) // Record success metrics if b.otel != nil { @@ -434,9 +455,9 @@ func (b *WorkflowBuilder) Build() (*v1alpha1.Workflow, error) { b.otel.addSpanAttributes(ctx, attribute.String("workflow.name", b.namePrefix), attribute.String("workflow.namespace", b.namespace), - attribute.Int("workflow.templates_count", len(templates)), - attribute.Int("workflow.steps_count", len(b.entryPoint)), - attribute.Bool("workflow.has_exit_handler", len(b.exitHandlers) > 0), + attribute.Int("workflow.templates_count", len(wf.Spec.Templates)), + attribute.Int("workflow.steps_count", len(entrySteps)), + attribute.Bool("workflow.has_exit_handler", onExit != ""), ) } @@ -463,7 +484,7 @@ func (b *WorkflowBuilder) Build() (*v1alpha1.Workflow, error) { // builder.AddTemplate(entryTemplate) // wf, err := builder.BuildWithEntrypoint("custom-main") func (b *WorkflowBuilder) BuildWithEntrypoint(entrypointName string) (*v1alpha1.Workflow, error) { - ctx := context.Background() + ctx := b.context() // Start tracing and timing startTime := time.Now() @@ -479,8 +500,7 @@ func (b *WorkflowBuilder) BuildWithEntrypoint(entrypointName string) (*v1alpha1. }() } - logger := otel.NewLogHelper(ctx, b.otelConfig, - "github.com/jasoet/pkg/v2/argo/builder", "WorkflowBuilder.BuildWithEntrypoint") + logger := b.newLogger(ctx, "WorkflowBuilder.BuildWithEntrypoint") logger.Debug("Building workflow with custom entrypoint", otel.F("name", b.namePrefix), otel.F("namespace", b.namespace), @@ -488,12 +508,12 @@ func (b *WorkflowBuilder) BuildWithEntrypoint(entrypointName string) (*v1alpha1. otel.F("templates_count", len(b.templates))) // Check for errors - if len(b.errors) > 0 { + if err := b.joinedError(); err != nil { if b.otel != nil { - b.otel.recordError(ctx, "build_validation_error", b.errors[0]) + b.otel.recordError(ctx, "build_validation_error", err) } - logger.Error(b.errors[0], "Failed to build workflow") - return nil, b.errors[0] + logger.Error(err, "Failed to build workflow") + return nil, err } // Verify entrypoint template exists @@ -505,7 +525,7 @@ func (b *WorkflowBuilder) BuildWithEntrypoint(entrypointName string) (*v1alpha1. } } if !found { - err := fmt.Errorf("entrypoint template '%s' not found in templates", entrypointName) + err := fmt.Errorf("%w: %q", ErrEntrypointNotFound, entrypointName) if b.otel != nil { b.otel.recordError(ctx, "build_validation_error", err) } @@ -520,45 +540,16 @@ func (b *WorkflowBuilder) BuildWithEntrypoint(entrypointName string) (*v1alpha1. // Create exit handler template if needed const exitHandlerName = "exit-handler" var onExit string - if len(b.exitHandlers) > 0 { - exitHandler := v1alpha1.Template{ + exitSteps := b.orderedExitHandlers() + if len(exitSteps) > 0 { + templates = append(templates, v1alpha1.Template{ Name: exitHandlerName, - Steps: b.exitHandlers, - } - templates = append(templates, exitHandler) + Steps: exitSteps, + }) onExit = exitHandlerName } - // Build workflow - wf := &v1alpha1.Workflow{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: b.namePrefix, - Namespace: b.namespace, - Labels: b.labels, - Annotations: b.annotations, - }, - Spec: v1alpha1.WorkflowSpec{ - Entrypoint: entrypointName, - ServiceAccountName: b.serviceAccount, - Templates: templates, - Volumes: b.volumes, - Metrics: b.metrics, - ArchiveLogs: b.archiveLogs, - PodGC: b.podGC, - TTLStrategy: b.ttl, - ActiveDeadlineSeconds: b.activeDeadlineSeconds, - OnExit: onExit, - }, - } - - // Apply default retry strategy if set - if b.retryStrategy != nil { - for i := range wf.Spec.Templates { - if wf.Spec.Templates[i].RetryStrategy == nil { - wf.Spec.Templates[i].RetryStrategy = b.retryStrategy - } - } - } + wf := b.assembleWorkflow(entrypointName, onExit, templates) // Record success metrics if b.otel != nil { @@ -567,8 +558,8 @@ func (b *WorkflowBuilder) BuildWithEntrypoint(entrypointName string) (*v1alpha1. attribute.String("workflow.name", b.namePrefix), attribute.String("workflow.namespace", b.namespace), attribute.String("workflow.entrypoint", entrypointName), - attribute.Int("workflow.templates_count", len(templates)), - attribute.Bool("workflow.has_exit_handler", len(b.exitHandlers) > 0), + attribute.Int("workflow.templates_count", len(wf.Spec.Templates)), + attribute.Bool("workflow.has_exit_handler", onExit != ""), ) } @@ -598,10 +589,141 @@ func (b *WorkflowBuilder) AddTemplate(template v1alpha1.Template) *WorkflowBuild return b } -// insertTemplate adds a template to the workflow, deduplicating by name. +// insertTemplate adds a template to the workflow, deduplicating by name. Adding the same +// template (identical content) more than once is a no-op. Adding a DIFFERENT template under +// a name that is already taken records an ErrTemplateConflict error — silently dropping the +// second definition would otherwise make a step run the wrong image or command. func (b *WorkflowBuilder) insertTemplate(t v1alpha1.Template) { - if _, exists := b.uniqueTemplates[t.Name]; !exists { - b.templates = append(b.templates, t) - b.uniqueTemplates[t.Name] = struct{}{} + if _, exists := b.uniqueTemplates[t.Name]; exists { + if existing := b.findTemplate(t.Name); existing != nil && !reflect.DeepEqual(*existing, t) { + b.errors = append(b.errors, fmt.Errorf("%w: %q", ErrTemplateConflict, t.Name)) + } + return + } + b.templates = append(b.templates, t) + b.uniqueTemplates[t.Name] = struct{}{} +} + +// findTemplate returns a pointer to the already-registered template with the given name, +// or nil if none exists. +func (b *WorkflowBuilder) findTemplate(name string) *v1alpha1.Template { + for i := range b.templates { + if b.templates[i].Name == name { + return &b.templates[i] + } + } + return nil +} + +// joinedError aggregates every accumulated error into a single error via errors.Join, so +// callers see all build problems (not just the first) and can match any of them with +// errors.Is. Returns nil when no errors were recorded. +func (b *WorkflowBuilder) joinedError() error { + if len(b.errors) == 0 { + return nil + } + return errors.Join(b.errors...) +} + +// isPriorityExitStep reports whether an exit-handler step should run before other exit +// steps (cleanup/teardown ordering). +func isPriorityExitStep(name string) bool { + return strings.Contains(name, "destroy") || strings.Contains(name, "cleanup") +} + +// orderedExitHandlers returns priority exit steps followed by normal exit steps, each in +// insertion order. +func (b *WorkflowBuilder) orderedExitHandlers() []v1alpha1.ParallelSteps { + if len(b.exitHandlersPriority) == 0 { + return b.exitHandlers + } + out := make([]v1alpha1.ParallelSteps, 0, len(b.exitHandlersPriority)+len(b.exitHandlers)) + out = append(out, b.exitHandlersPriority...) + out = append(out, b.exitHandlers...) + return out +} + +// noopStep returns a step referencing an inserted no-op template. The template is added to +// the builder (deduplicated) as a side effect so the generated workflow references a real +// template. +func (b *WorkflowBuilder) noopStep() v1alpha1.WorkflowStep { + const noopTemplateName = "noop-template" + b.insertTemplate(v1alpha1.Template{ + Name: noopTemplateName, + Container: &corev1.Container{ + Image: "alpine:3.19", + Command: []string{"sh", "-c"}, + Args: []string{"echo noop"}, + }, + }) + return v1alpha1.WorkflowStep{Name: "noop", Template: noopTemplateName} +} + +// assembleWorkflow constructs the final Workflow. It deep-copies builder-owned state +// (labels, annotations, volumes, and every template) so a returned workflow can be mutated +// freely without affecting the builder or any other workflow produced by it. The default +// retry strategy, when set, is applied only to leaf templates (those without their own +// Steps) so it never wraps the generated entrypoint or exit-handler orchestration +// templates — which would otherwise re-run already-succeeded steps. +func (b *WorkflowBuilder) assembleWorkflow(entrypointName, onExit string, templates []v1alpha1.Template) *v1alpha1.Workflow { + // Deep-copy templates so their internal pointers (Container, Script, RetryStrategy, ...) + // are not aliased with builder-owned state. + copiedTemplates := make([]v1alpha1.Template, len(templates)) + for i := range templates { + copiedTemplates[i] = *templates[i].DeepCopy() + } + + // Apply the default retry strategy to leaf templates only. + if b.retryStrategy != nil { + for i := range copiedTemplates { + if copiedTemplates[i].RetryStrategy == nil && len(copiedTemplates[i].Steps) == 0 && copiedTemplates[i].DAG == nil { + copiedTemplates[i].RetryStrategy = b.retryStrategy.DeepCopy() + } + } + } + + return &v1alpha1.Workflow{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: b.namePrefix, + Namespace: b.namespace, + Labels: copyStringMap(b.labels), + Annotations: copyStringMap(b.annotations), + }, + Spec: v1alpha1.WorkflowSpec{ + Entrypoint: entrypointName, + ServiceAccountName: b.serviceAccount, + Templates: copiedTemplates, + Volumes: copyVolumes(b.volumes), + Metrics: b.metrics, + ArchiveLogs: b.archiveLogs, + PodGC: b.podGC, + TTLStrategy: b.ttl, + ActiveDeadlineSeconds: b.activeDeadlineSeconds, + OnExit: onExit, + }, + } +} + +// copyStringMap returns a shallow copy of a string map, or nil if the input is nil. +func copyStringMap(m map[string]string) map[string]string { + if m == nil { + return nil + } + out := make(map[string]string, len(m)) + for k, v := range m { + out[k] = v + } + return out +} + +// copyVolumes returns a deep copy of the volumes slice, or nil if the input is empty. +func copyVolumes(vols []corev1.Volume) []corev1.Volume { + if len(vols) == 0 { + return nil + } + out := make([]corev1.Volume, len(vols)) + for i := range vols { + out[i] = *vols[i].DeepCopy() } + return out } diff --git a/argo/builder/builder_fixes_test.go b/argo/builder/builder_fixes_test.go new file mode 100644 index 0000000..7cc1785 --- /dev/null +++ b/argo/builder/builder_fixes_test.go @@ -0,0 +1,230 @@ +package builder + +import ( + "context" + "io" + "os" + "testing" + + "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + sdktrace "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/sdk/trace/tracetest" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/intstr" + + "github.com/jasoet/pkg/v3/argo/builder/template" + "github.com/jasoet/pkg/v3/otel" +) + +func findTemplateByName(wf *v1alpha1.Workflow, name string) *v1alpha1.Template { + for i := range wf.Spec.Templates { + if wf.Spec.Templates[i].Name == name { + return &wf.Spec.Templates[i] + } + } + return nil +} + +// TestBuild_DefaultRetryStrategy_SkipsOrchestrationTemplates verifies the default retry +// strategy is applied to leaf templates only — never to the generated "main" entrypoint or +// the "exit-handler" steps template. Retrying an orchestration template would re-run +// already-succeeded steps (deploys, payments). +func TestBuild_DefaultRetryStrategy_SkipsOrchestrationTemplates(t *testing.T) { + limit := intstr.FromInt32(3) + retry := &v1alpha1.RetryStrategy{Limit: &limit, RetryPolicy: "Always"} + + wf, err := NewWorkflowBuilder("test", "argo", WithRetryStrategy(retry)). + Add(template.NewContainer("deploy", "app:v1", template.WithCommand("deploy.sh"))). + AddExitHandler(template.NewContainer("cleanup", "app:v1", template.WithCommand("cleanup.sh"))). + Build() + require.NoError(t, err) + + mainTmpl := findTemplateByName(wf, "main") + require.NotNil(t, mainTmpl) + assert.Nil(t, mainTmpl.RetryStrategy, "main (entrypoint steps) template must NOT get the default retry strategy") + + exitTmpl := findTemplateByName(wf, "exit-handler") + require.NotNil(t, exitTmpl) + assert.Nil(t, exitTmpl.RetryStrategy, "exit-handler steps template must NOT get the default retry strategy") + + leaf := findTemplateByName(wf, "deploy-template") + require.NotNil(t, leaf) + require.NotNil(t, leaf.RetryStrategy, "leaf container template SHOULD get the default retry strategy") + assert.Equal(t, 3, leaf.RetryStrategy.Limit.IntValue()) +} + +// TestBuild_DeepCopiesBuilderState verifies a built workflow does not alias builder-owned +// maps/slices/pointers, so mutating one workflow affects neither the builder nor another +// workflow produced by the same builder. +func TestBuild_DeepCopiesBuilderState(t *testing.T) { + b := NewWorkflowBuilder("test", "argo", + WithLabels(map[string]string{"app": "orig"}), + WithAnnotations(map[string]string{"note": "orig"}), + WithVolume(corev1.Volume{Name: "data", VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}}), + ).Add(template.NewContainer("step", "alpine:latest", template.WithCommand("echo", "hi"))) + + wf1, err := b.Build() + require.NoError(t, err) + wf2, err := b.Build() + require.NoError(t, err) + + // Mutate wf1's owned state. + wf1.Labels["app"] = "mutated" + wf1.Annotations["note"] = "mutated" + wf1.Spec.Volumes[0].Name = "mutated" + if tmpl := findTemplateByName(wf1, "step-template"); tmpl != nil && tmpl.Container != nil { + tmpl.Container.Command[0] = "mutated" + } + + // wf2 must be unaffected. + assert.Equal(t, "orig", wf2.Labels["app"]) + assert.Equal(t, "orig", wf2.Annotations["note"]) + assert.Equal(t, "data", wf2.Spec.Volumes[0].Name) + tmpl2 := findTemplateByName(wf2, "step-template") + require.NotNil(t, tmpl2) + assert.Equal(t, "echo", tmpl2.Container.Command[0]) + + // Builder-owned state must be unaffected too. + assert.Equal(t, "orig", b.labels["app"]) + assert.Equal(t, "orig", b.annotations["note"]) + assert.Equal(t, "data", b.volumes[0].Name) +} + +// TestBuild_EmptyInsertsNoop verifies an empty builder yields a valid workflow with a +// no-op step (not a zero-step entrypoint that the server rejects). +func TestBuild_EmptyInsertsNoop(t *testing.T) { + wf, err := NewWorkflowBuilder("empty", "argo").Build() + require.NoError(t, err) + + mainTmpl := findTemplateByName(wf, "main") + require.NotNil(t, mainTmpl) + require.Len(t, mainTmpl.Steps, 1) + require.Len(t, mainTmpl.Steps[0].Steps, 1) + assert.Equal(t, "noop", mainTmpl.Steps[0].Steps[0].Name) + + noopTmpl := findTemplateByName(wf, "noop-template") + require.NotNil(t, noopTmpl, "noop leaf template must be inserted") + require.NotNil(t, noopTmpl.Container) +} + +// TestInsertTemplate_ConflictRecordsError verifies that adding a DIFFERENT template under +// an existing name records an ErrTemplateConflict rather than silently dropping it. +func TestInsertTemplate_ConflictRecordsError(t *testing.T) { + tmplA := v1alpha1.Template{Name: "dup", Container: &corev1.Container{Image: "a:1"}} + tmplB := v1alpha1.Template{Name: "dup", Container: &corev1.Container{Image: "b:2"}} + + _, err := NewWorkflowBuilder("test", "argo"). + AddTemplate(tmplA). + AddTemplate(tmplB). + Add(template.NewContainer("step", "alpine:latest")). + Build() + + require.Error(t, err) + assert.ErrorIs(t, err, ErrTemplateConflict) + assert.Contains(t, err.Error(), "dup") +} + +// TestInsertTemplate_IdenticalDuplicateIsNoError verifies identical re-adds are silently +// deduplicated (patterns legitimately add the same template many times). +func TestInsertTemplate_IdenticalDuplicateIsNoError(t *testing.T) { + tmpl := v1alpha1.Template{Name: "same", Container: &corev1.Container{Image: "a:1"}} + + wf, err := NewWorkflowBuilder("test", "argo"). + AddTemplate(tmpl). + AddTemplate(tmpl). + Add(template.NewContainer("step", "alpine:latest")). + Build() + + require.NoError(t, err) + assert.NotNil(t, findTemplateByName(wf, "same")) +} + +// TestBuild_JoinsAllErrors verifies Build aggregates every accumulated error (not just the +// first) via errors.Join. +func TestBuild_JoinsAllErrors(t *testing.T) { + _, err := NewWorkflowBuilder("test", "argo"). + AddTemplate(v1alpha1.Template{Name: "x", Container: &corev1.Container{Image: "a:1"}}). + AddTemplate(v1alpha1.Template{Name: "x", Container: &corev1.Container{Image: "b:2"}}). + AddTemplate(v1alpha1.Template{Name: "y", Container: &corev1.Container{Image: "a:1"}}). + AddTemplate(v1alpha1.Template{Name: "y", Container: &corev1.Container{Image: "c:3"}}). + Build() + + require.Error(t, err) + assert.ErrorIs(t, err, ErrTemplateConflict) + // Both conflicting names should appear in the joined error. + assert.Contains(t, err.Error(), "x") + assert.Contains(t, err.Error(), "y") +} + +// TestAddExitHandler_PreservesOrder verifies that multiple cleanup steps keep their +// insertion order (they are no longer reversed by repeated prepending) and still run +// before normal exit steps. +func TestAddExitHandler_PreservesOrder(t *testing.T) { + wf, err := NewWorkflowBuilder("test", "argo"). + Add(template.NewContainer("main-step", "alpine:latest", template.WithCommand("echo", "hi"))). + AddExitHandler(template.NewContainer("cleanup-a", "alpine:latest", template.WithCommand("echo", "a"))). + AddExitHandler(template.NewContainer("cleanup-b", "alpine:latest", template.WithCommand("echo", "b"))). + AddExitHandler(template.NewContainer("notify", "alpine:latest", template.WithCommand("echo", "n"))). + Build() + require.NoError(t, err) + + exit := findTemplateByName(wf, "exit-handler") + require.NotNil(t, exit) + + var order []string + for _, ps := range exit.Steps { + for _, s := range ps.Steps { + order = append(order, s.Name) + } + } + // Priority cleanup steps first (in insertion order a,b), then normal steps. + assert.Equal(t, []string{"cleanup-a", "cleanup-b", "notify"}, order) +} + +// TestBuild_NoStderrLogsWithoutOTelConfig verifies that, without an OTel config, the +// builder does not emit unleveled Debug/Info logs to stderr. +func TestBuild_NoStderrLogsWithoutOTelConfig(t *testing.T) { + old := os.Stderr + r, w, err := os.Pipe() + require.NoError(t, err) + os.Stderr = w + t.Cleanup(func() { os.Stderr = old }) + + _, buildErr := NewWorkflowBuilder("quiet", "argo"). + Add(template.NewContainer("step", "alpine:latest", template.WithCommand("echo", "hi"))). + Build() + require.NoError(t, buildErr) + + require.NoError(t, w.Close()) + os.Stderr = old + out, err := io.ReadAll(r) + require.NoError(t, err) + assert.Empty(t, string(out), "no Debug/Info stderr output expected when OTel config is absent") +} + +// TestBuild_WithContext_RootsSpansAtParent verifies WithContext threads a parent context so +// builder spans are children of the caller's trace rather than orphan roots. +func TestBuild_WithContext_RootsSpansAtParent(t *testing.T) { + exporter := tracetest.NewInMemoryExporter() + tp := sdktrace.NewTracerProvider(sdktrace.WithSyncer(exporter)) + t.Cleanup(func() { _ = tp.Shutdown(context.Background()) }) + + cfg := otel.NewConfig("test", otel.WithTracerProvider(tp)) + parentCtx, parentSpan := tp.Tracer("test").Start(context.Background(), "parent") + parentTraceID := parentSpan.SpanContext().TraceID() + + _, err := NewWorkflowBuilder("ctx", "argo", WithContext(parentCtx), WithOTelConfig(cfg)). + Add(template.NewContainer("step", "alpine:latest", template.WithCommand("echo", "hi"))). + Build() + require.NoError(t, err) + parentSpan.End() + + spans := exporter.GetSpans() + require.NotEmpty(t, spans) + for _, s := range spans { + assert.Equal(t, parentTraceID, s.SpanContext.TraceID(), + "builder span %q must share the parent trace, not start a new root", s.Name) + } +} diff --git a/argo/builder/builder_test.go b/argo/builder/builder_test.go index c988db2..a4206c7 100644 --- a/argo/builder/builder_test.go +++ b/argo/builder/builder_test.go @@ -8,7 +8,7 @@ import ( "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" - "github.com/jasoet/pkg/v2/argo/builder/template" + "github.com/jasoet/pkg/v3/argo/builder/template" ) func TestWorkflowBuilder_Build(t *testing.T) { @@ -422,7 +422,8 @@ func TestWorkflowBuilder_BuildWithEntrypoint(t *testing.T) { wf, err := builder.BuildWithEntrypoint("nonexistent") require.Error(t, err) assert.Nil(t, wf) - assert.Contains(t, err.Error(), "entrypoint template 'nonexistent' not found") + assert.ErrorIs(t, err, ErrEntrypointNotFound) + assert.Contains(t, err.Error(), "nonexistent") }) t.Run("builds with exit handler", func(t *testing.T) { diff --git a/argo/builder/errors.go b/argo/builder/errors.go new file mode 100644 index 0000000..638e442 --- /dev/null +++ b/argo/builder/errors.go @@ -0,0 +1,22 @@ +package builder + +import "errors" + +// Sentinel errors returned by the workflow builder. Callers can match them with +// errors.Is to branch on failure modes without string comparison. Build() aggregates +// all accumulated errors with errors.Join, so a returned error may match more than one +// of these (and may also wrap errors from the underlying WorkflowSource implementations). +var ( + // ErrTemplateConflict is returned when two templates share the same name but differ + // in content. The second, differing definition is dropped (the first one wins), which + // would otherwise silently make a step run the wrong image or command. + ErrTemplateConflict = errors.New("argo/builder: conflicting templates with the same name") + + // ErrEntrypointNotFound is returned by BuildWithEntrypoint when the named entrypoint + // template has not been added to the builder. + ErrEntrypointNotFound = errors.New("argo/builder: entrypoint template not found") + + // ErrTemplateSource is returned when a WorkflowSource fails to produce its steps or + // templates during Add, AddParallel, or AddExitHandler. + ErrTemplateSource = errors.New("argo/builder: workflow source error") +) diff --git a/argo/builder/option.go b/argo/builder/option.go index 5353502..c7f2740 100644 --- a/argo/builder/option.go +++ b/argo/builder/option.go @@ -1,23 +1,44 @@ package builder import ( + "context" + "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1" corev1 "k8s.io/api/core/v1" - "github.com/jasoet/pkg/v2/otel" + "github.com/jasoet/pkg/v3/otel" ) // Option is a functional option for configuring WorkflowBuilder. type Option func(*WorkflowBuilder) +// WithContext sets the parent context used to root the builder's OpenTelemetry trace +// spans (for Add, AddParallel, AddExitHandler, Build, and BuildWithEntrypoint). Provide the +// caller's request context so builder spans become children of the active trace instead of +// orphan root spans. Defaults to context.Background(). +// +// Example: +// +// builder := NewWorkflowBuilder("my-workflow", "argo", +// WithContext(ctx), +// WithOTelConfig(otelConfig)) +func WithContext(ctx context.Context) Option { + return func(b *WorkflowBuilder) { + if ctx != nil { + b.baseCtx = ctx + } + } +} + // WithOTelConfig enables OpenTelemetry instrumentation for the workflow builder. // This adds distributed tracing, metrics collection, and structured logging to workflow build operations. // // Example: // -// otelConfig := otel.NewConfig("workflow-service"). -// WithTracerProvider(tp). -// WithMeterProvider(mp) +// otelConfig := otel.NewConfig("workflow-service", +// otel.WithTracerProvider(tp), +// otel.WithMeterProvider(mp), +// ) // builder := NewWorkflowBuilder("my-workflow", "argo", // WithOTelConfig(otelConfig)) func WithOTelConfig(cfg *otel.Config) Option { @@ -42,14 +63,21 @@ func WithServiceAccount(sa string) Option { // WithRetryStrategy sets a default retry strategy for all workflow steps. // Individual steps can override this with their own retry configuration. // -// Example: +// The default retry strategy is applied only to leaf templates (Container/Script/HTTP/...), +// never to the generated entrypoint or exit-handler step templates — retrying an +// orchestration template would re-run already-succeeded steps. +// +// Note: RetryStrategy.Limit and Backoff.Factor are *intstr.IntOrString, so take the +// address of an intstr value: // +// limit := intstr.FromInt32(3) +// factor := intstr.FromInt32(2) // retryStrategy := &v1alpha1.RetryStrategy{ -// Limit: intstr.FromInt(3), +// Limit: &limit, // RetryPolicy: "Always", // Backoff: &v1alpha1.Backoff{ -// Duration: "1m", -// Factor: intstr.FromInt(2), +// Duration: "1m", +// Factor: &factor, // MaxDuration: "10m", // }, // } diff --git a/argo/builder/option_test.go b/argo/builder/option_test.go index 7350df8..bb3a6e9 100644 --- a/argo/builder/option_test.go +++ b/argo/builder/option_test.go @@ -9,7 +9,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/intstr" - "github.com/jasoet/pkg/v2/otel" + "github.com/jasoet/pkg/v3/otel" ) func TestWithOTelConfig(t *testing.T) { diff --git a/argo/builder/otel.go b/argo/builder/otel.go index 5afb7f3..363f5da 100644 --- a/argo/builder/otel.go +++ b/argo/builder/otel.go @@ -7,9 +7,13 @@ import ( "go.opentelemetry.io/otel/metric" "go.opentelemetry.io/otel/trace" - "github.com/jasoet/pkg/v2/otel" + "github.com/jasoet/pkg/v3/otel" ) +// instrumentationVersion is the OpenTelemetry instrumentation-scope version reported by the +// builder's tracer and meter. It tracks the module major version (v3). +const instrumentationVersion = "v3.0.0" + // otelInstrumentation holds OpenTelemetry instrumentation components for the workflow builder. // It provides tracing, metrics, and logging capabilities for workflow build operations. type otelInstrumentation struct { @@ -43,16 +47,16 @@ func newOTelInstrumentation(cfg *otel.Config) *otelInstrumentation { // Get tracer for distributed tracing if cfg.TracerProvider != nil { inst.tracer = cfg.TracerProvider.Tracer( - "github.com/jasoet/pkg/v2/argo/builder", - trace.WithInstrumentationVersion("v2.0.0"), + "github.com/jasoet/pkg/v3/argo/builder", + trace.WithInstrumentationVersion(instrumentationVersion), ) } // Get meter and create metrics if cfg.MeterProvider != nil { inst.meter = cfg.MeterProvider.Meter( - "github.com/jasoet/pkg/v2/argo/builder", - metric.WithInstrumentationVersion("v2.0.0"), + "github.com/jasoet/pkg/v3/argo/builder", + metric.WithInstrumentationVersion(instrumentationVersion), ) // Create counter metrics (errors intentionally ignored - metrics are optional) diff --git a/argo/builder/otel_test.go b/argo/builder/otel_test.go index 3d5bc9e..baae1bc 100644 --- a/argo/builder/otel_test.go +++ b/argo/builder/otel_test.go @@ -13,9 +13,31 @@ import ( noopt "go.opentelemetry.io/otel/sdk/trace" "go.opentelemetry.io/otel/sdk/trace/tracetest" - "github.com/jasoet/pkg/v2/otel" + "github.com/jasoet/pkg/v3/otel" ) +// sumInt64Counter returns the summed value of all data points for the named int64 counter +// across the collected metrics, and whether the metric was found. +func sumInt64Counter(rm metricdata.ResourceMetrics, name string) (int64, bool) { + for _, sm := range rm.ScopeMetrics { + for _, m := range sm.Metrics { + if m.Name != name { + continue + } + sum, ok := m.Data.(metricdata.Sum[int64]) + if !ok { + return 0, false + } + var total int64 + for _, dp := range sum.DataPoints { + total += dp.Value + } + return total, true + } + } + return 0, false +} + func TestNewOTelInstrumentation(t *testing.T) { t.Run("creates disabled instrumentation with nil config", func(t *testing.T) { inst := newOTelInstrumentation(nil) @@ -29,9 +51,9 @@ func TestNewOTelInstrumentation(t *testing.T) { tracerProvider := noopt.NewTracerProvider() meterProvider := sdkmetric.NewMeterProvider() - cfg := otel.NewConfig("test-service"). - WithTracerProvider(tracerProvider). - WithMeterProvider(meterProvider) + cfg := otel.NewConfig("test-service", + otel.WithTracerProvider(tracerProvider), + otel.WithMeterProvider(meterProvider)) inst := newOTelInstrumentation(cfg) require.NotNil(t, inst) @@ -46,8 +68,8 @@ func TestNewOTelInstrumentation(t *testing.T) { t.Run("creates instrumentation with tracer only", func(t *testing.T) { tracerProvider := noopt.NewTracerProvider() - cfg := otel.NewConfig("test-service"). - WithTracerProvider(tracerProvider) + cfg := otel.NewConfig("test-service", + otel.WithTracerProvider(tracerProvider)) inst := newOTelInstrumentation(cfg) require.NotNil(t, inst) @@ -58,8 +80,8 @@ func TestNewOTelInstrumentation(t *testing.T) { t.Run("creates instrumentation with meter only", func(t *testing.T) { meterProvider := sdkmetric.NewMeterProvider() - cfg := otel.NewConfig("test-service"). - WithMeterProvider(meterProvider) + cfg := otel.NewConfig("test-service", + otel.WithMeterProvider(meterProvider)) inst := newOTelInstrumentation(cfg) require.NotNil(t, inst) @@ -95,8 +117,8 @@ func TestStartSpan(t *testing.T) { noopt.WithSyncer(exporter), ) - cfg := otel.NewConfig("test-service"). - WithTracerProvider(tracerProvider) + cfg := otel.NewConfig("test-service", + otel.WithTracerProvider(tracerProvider)) inst := newOTelInstrumentation(cfg) ctx := context.Background() @@ -135,9 +157,9 @@ func TestRecordError(t *testing.T) { sdkmetric.WithReader(reader), ) - cfg := otel.NewConfig("test-service"). - WithTracerProvider(tracerProvider). - WithMeterProvider(meterProvider) + cfg := otel.NewConfig("test-service", + otel.WithTracerProvider(tracerProvider), + otel.WithMeterProvider(meterProvider)) inst := newOTelInstrumentation(cfg) ctx := context.Background() @@ -176,8 +198,8 @@ func TestIncrementCounter(t *testing.T) { sdkmetric.WithReader(reader), ) - cfg := otel.NewConfig("test-service"). - WithMeterProvider(meterProvider) + cfg := otel.NewConfig("test-service", + otel.WithMeterProvider(meterProvider)) inst := newOTelInstrumentation(cfg) ctx := context.Background() @@ -188,6 +210,10 @@ func TestIncrementCounter(t *testing.T) { var rm metricdata.ResourceMetrics err := reader.Collect(ctx, &rm) require.NoError(t, err) + + total, found := sumInt64Counter(rm, "argo.workflows.built") + require.True(t, found, "argo.workflows.built counter should be recorded") + assert.Equal(t, int64(3), total, "two increments of 1 and 2 should sum to 3") }) t.Run("increments templates_added counter", func(t *testing.T) { @@ -196,8 +222,8 @@ func TestIncrementCounter(t *testing.T) { sdkmetric.WithReader(reader), ) - cfg := otel.NewConfig("test-service"). - WithMeterProvider(meterProvider) + cfg := otel.NewConfig("test-service", + otel.WithMeterProvider(meterProvider)) inst := newOTelInstrumentation(cfg) ctx := context.Background() @@ -207,6 +233,10 @@ func TestIncrementCounter(t *testing.T) { var rm metricdata.ResourceMetrics err := reader.Collect(ctx, &rm) require.NoError(t, err) + + total, found := sumInt64Counter(rm, "argo.workflows.templates_added") + require.True(t, found, "argo.workflows.templates_added counter should be recorded") + assert.Equal(t, int64(5), total) }) t.Run("increments sources_added counter", func(t *testing.T) { @@ -215,8 +245,8 @@ func TestIncrementCounter(t *testing.T) { sdkmetric.WithReader(reader), ) - cfg := otel.NewConfig("test-service"). - WithMeterProvider(meterProvider) + cfg := otel.NewConfig("test-service", + otel.WithMeterProvider(meterProvider)) inst := newOTelInstrumentation(cfg) ctx := context.Background() @@ -226,6 +256,10 @@ func TestIncrementCounter(t *testing.T) { var rm metricdata.ResourceMetrics err := reader.Collect(ctx, &rm) require.NoError(t, err) + + total, found := sumInt64Counter(rm, "argo.workflows.sources_added") + require.True(t, found, "argo.workflows.sources_added counter should be recorded") + assert.Equal(t, int64(3), total) }) t.Run("increments counter with attributes", func(t *testing.T) { @@ -234,8 +268,8 @@ func TestIncrementCounter(t *testing.T) { sdkmetric.WithReader(reader), ) - cfg := otel.NewConfig("test-service"). - WithMeterProvider(meterProvider) + cfg := otel.NewConfig("test-service", + otel.WithMeterProvider(meterProvider)) inst := newOTelInstrumentation(cfg) ctx := context.Background() @@ -247,6 +281,10 @@ func TestIncrementCounter(t *testing.T) { var rm metricdata.ResourceMetrics err := reader.Collect(ctx, &rm) require.NoError(t, err) + + total, found := sumInt64Counter(rm, "argo.workflows.built") + require.True(t, found, "argo.workflows.built counter should be recorded") + assert.Equal(t, int64(1), total) }) t.Run("handles unknown counter name", func(t *testing.T) { @@ -255,8 +293,8 @@ func TestIncrementCounter(t *testing.T) { sdkmetric.WithReader(reader), ) - cfg := otel.NewConfig("test-service"). - WithMeterProvider(meterProvider) + cfg := otel.NewConfig("test-service", + otel.WithMeterProvider(meterProvider)) inst := newOTelInstrumentation(cfg) ctx := context.Background() @@ -281,8 +319,8 @@ func TestRecordDuration(t *testing.T) { sdkmetric.WithReader(reader), ) - cfg := otel.NewConfig("test-service"). - WithMeterProvider(meterProvider) + cfg := otel.NewConfig("test-service", + otel.WithMeterProvider(meterProvider)) inst := newOTelInstrumentation(cfg) ctx := context.Background() @@ -293,6 +331,27 @@ func TestRecordDuration(t *testing.T) { var rm metricdata.ResourceMetrics err := reader.Collect(ctx, &rm) require.NoError(t, err) + + var found bool + for _, sm := range rm.ScopeMetrics { + for _, m := range sm.Metrics { + if m.Name != "argo.workflows.build_duration" { + continue + } + hist, ok := m.Data.(metricdata.Histogram[float64]) + require.True(t, ok, "build_duration should be a float64 histogram") + var count uint64 + var sum float64 + for _, dp := range hist.DataPoints { + count += dp.Count + sum += dp.Sum + } + assert.Equal(t, uint64(2), count, "two durations recorded") + assert.InDelta(t, 358.01, sum, 0.001, "histogram sum should equal the recorded durations") + found = true + } + } + require.True(t, found, "argo.workflows.build_duration histogram should be recorded") }) t.Run("records duration with attributes", func(t *testing.T) { @@ -301,8 +360,8 @@ func TestRecordDuration(t *testing.T) { sdkmetric.WithReader(reader), ) - cfg := otel.NewConfig("test-service"). - WithMeterProvider(meterProvider) + cfg := otel.NewConfig("test-service", + otel.WithMeterProvider(meterProvider)) inst := newOTelInstrumentation(cfg) ctx := context.Background() @@ -322,8 +381,8 @@ func TestRecordDuration(t *testing.T) { sdkmetric.WithReader(reader), ) - cfg := otel.NewConfig("test-service"). - WithMeterProvider(meterProvider) + cfg := otel.NewConfig("test-service", + otel.WithMeterProvider(meterProvider)) inst := newOTelInstrumentation(cfg) ctx := context.Background() @@ -348,8 +407,8 @@ func TestAddSpanAttributes(t *testing.T) { noopt.WithSyncer(exporter), ) - cfg := otel.NewConfig("test-service"). - WithTracerProvider(tracerProvider) + cfg := otel.NewConfig("test-service", + otel.WithTracerProvider(tracerProvider)) inst := newOTelInstrumentation(cfg) ctx := context.Background() @@ -371,8 +430,8 @@ func TestAddSpanAttributes(t *testing.T) { }) t.Run("does nothing when no active span", func(t *testing.T) { - cfg := otel.NewConfig("test-service"). - WithTracerProvider(noopt.NewTracerProvider()) + cfg := otel.NewConfig("test-service", + otel.WithTracerProvider(noopt.NewTracerProvider())) inst := newOTelInstrumentation(cfg) ctx := context.Background() diff --git a/argo/builder/template/container.go b/argo/builder/template/container.go index 976b227..16a9aec 100644 --- a/argo/builder/template/container.go +++ b/argo/builder/template/container.go @@ -8,7 +8,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" - "github.com/jasoet/pkg/v2/otel" + "github.com/jasoet/pkg/v3/otel" ) // Container is a WorkflowSource that creates a container-based workflow step. @@ -73,13 +73,16 @@ func NewContainer(name, image string, opts ...ContainerOption) *Container { return c } -// Command sets the container command (entrypoint override). -// Can be called multiple times or with multiple arguments. +// Command appends to the container command (entrypoint override). Successive calls +// accumulate, so Command("python").Command("app.py") yields ["python", "app.py"]. +// +// Note: this differs from the WithCommand option, which REPLACES the command. Use the +// option to set an initial command at construction; use this method to extend it. // // Example: // // container.Command("python", "app.py") -// // or +// // or, appending across calls // container.Command("python").Command("app.py") func (c *Container) Command(cmd ...string) *Container { c.command = append(c.command, cmd...) @@ -219,10 +222,11 @@ func (c *Container) ContinueOn(continueOn *v1alpha1.ContinueOn) *Container { // WithRetry sets a retry strategy for this specific step. // -// Example: +// RetryStrategy.Limit is *intstr.IntOrString, so take the address of an intstr value: // +// limit := intstr.FromInt32(3) // container.WithRetry(&v1alpha1.RetryStrategy{ -// Limit: intstr.FromInt(3), +// Limit: &limit, // RetryPolicy: "Always", // }) func (c *Container) WithRetry(retry *v1alpha1.RetryStrategy) *Container { @@ -235,7 +239,7 @@ func (c *Container) Steps() ([]v1alpha1.WorkflowStep, error) { ctx := context.Background() logger := otel.NewLogHelper(ctx, c.otelConfig, - "github.com/jasoet/pkg/v2/argo/builder/template", "Container.Steps") + "github.com/jasoet/pkg/v3/argo/builder/template", "Container.Steps") logger.Debug("Generating container steps", otel.F("name", c.name), otel.F("image", c.image)) @@ -263,7 +267,7 @@ func (c *Container) Templates() ([]v1alpha1.Template, error) { ctx := context.Background() logger := otel.NewLogHelper(ctx, c.otelConfig, - "github.com/jasoet/pkg/v2/argo/builder/template", "Container.Templates") + "github.com/jasoet/pkg/v3/argo/builder/template", "Container.Templates") logger.Debug("Generating container template", otel.F("name", c.templateName), otel.F("image", c.image)) diff --git a/argo/builder/template/container_test.go b/argo/builder/template/container_test.go index 6eff4e0..be254a6 100644 --- a/argo/builder/template/container_test.go +++ b/argo/builder/template/container_test.go @@ -11,7 +11,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" - "github.com/jasoet/pkg/v2/otel" + "github.com/jasoet/pkg/v3/otel" ) func TestNewContainer(t *testing.T) { diff --git a/argo/builder/template/http.go b/argo/builder/template/http.go index 9efd722..a5fb2ed 100644 --- a/argo/builder/template/http.go +++ b/argo/builder/template/http.go @@ -6,7 +6,7 @@ import ( "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1" - "github.com/jasoet/pkg/v2/otel" + "github.com/jasoet/pkg/v3/otel" ) // HTTP is a WorkflowSource that creates an HTTP request workflow step. @@ -134,12 +134,23 @@ func (h *HTTP) When(condition string) *HTTP { return h } +// ContinueOn configures the step to continue even when it fails or errors. Without this, +// the continueOn field read by Steps stays nil and the feature is unreachable. +// +// Example: +// +// http.ContinueOn(&v1alpha1.ContinueOn{Failed: true}) +func (h *HTTP) ContinueOn(continueOn *v1alpha1.ContinueOn) *HTTP { + h.continueOn = continueOn + return h +} + // Steps implements WorkflowSource interface. func (h *HTTP) Steps() ([]v1alpha1.WorkflowStep, error) { ctx := context.Background() logger := otel.NewLogHelper(ctx, h.otelConfig, - "github.com/jasoet/pkg/v2/argo/builder/template", "HTTP.Steps") + "github.com/jasoet/pkg/v3/argo/builder/template", "HTTP.Steps") logger.Debug("Generating HTTP steps", otel.F("name", h.name), otel.F("url", h.url), @@ -172,7 +183,7 @@ func (h *HTTP) Templates() ([]v1alpha1.Template, error) { ctx := context.Background() logger := otel.NewLogHelper(ctx, h.otelConfig, - "github.com/jasoet/pkg/v2/argo/builder/template", "HTTP.Templates") + "github.com/jasoet/pkg/v3/argo/builder/template", "HTTP.Templates") logger.Debug("Generating HTTP template", otel.F("name", h.templateName), otel.F("url", h.url)) @@ -256,3 +267,10 @@ func WithHTTPOTelConfig(cfg *otel.Config) HTTPOption { h.otelConfig = cfg } } + +// WithHTTPContinueOn configures the step to continue on failure/error. +func WithHTTPContinueOn(continueOn *v1alpha1.ContinueOn) HTTPOption { + return func(h *HTTP) { + h.continueOn = continueOn + } +} diff --git a/argo/builder/template/http_test.go b/argo/builder/template/http_test.go index 9a1509e..6085d93 100644 --- a/argo/builder/template/http_test.go +++ b/argo/builder/template/http_test.go @@ -254,3 +254,24 @@ func TestHTTPWithContinueOn(t *testing.T) { require.NotNil(t, step.ContinueOn) assert.True(t, step.ContinueOn.Failed) } + +func TestHTTP_ContinueOnSetters(t *testing.T) { + t.Run("method", func(t *testing.T) { + http := NewHTTP("check", WithHTTPURL("https://api.example.com")). + ContinueOn(&v1alpha1.ContinueOn{Failed: true}) + steps, err := http.Steps() + require.NoError(t, err) + require.NotNil(t, steps[0].ContinueOn) + assert.True(t, steps[0].ContinueOn.Failed) + }) + + t.Run("option", func(t *testing.T) { + http := NewHTTP("check", + WithHTTPURL("https://api.example.com"), + WithHTTPContinueOn(&v1alpha1.ContinueOn{Failed: true})) + steps, err := http.Steps() + require.NoError(t, err) + require.NotNil(t, steps[0].ContinueOn) + assert.True(t, steps[0].ContinueOn.Failed) + }) +} diff --git a/argo/builder/template/script.go b/argo/builder/template/script.go index e09cf13..346c44f 100644 --- a/argo/builder/template/script.go +++ b/argo/builder/template/script.go @@ -8,7 +8,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" - "github.com/jasoet/pkg/v2/otel" + "github.com/jasoet/pkg/v3/otel" ) // Script is a WorkflowSource that creates a script-based workflow step. @@ -37,6 +37,9 @@ type Script struct { continueOn *v1alpha1.ContinueOn retryStrategy *v1alpha1.RetryStrategy otelConfig *otel.Config + // langErr records an unknown-language error from NewScript so it can surface from + // Templates() (NewScript itself cannot return an error). + langErr error } // NewScript creates a new script workflow source. @@ -60,7 +63,10 @@ func NewScript(name, language string, opts ...ScriptOption) *Script { volumeMounts: make([]corev1.VolumeMount, 0), } - // Set default image based on language + // Set default image based on language. An unknown language records an error (surfaced + // from Templates) rather than silently defaulting to bash, which would run the source + // under the wrong interpreter. The bash default is kept only so the struct is usable if + // the caller later overrides the image/command explicitly. switch language { case "bash", "sh": s.image = "bash:5.2" @@ -75,9 +81,11 @@ func NewScript(name, language string, opts ...ScriptOption) *Script { s.image = "ruby:3.2-slim" s.command = []string{"ruby"} default: - // Default to bash s.image = "bash:5.2" s.command = []string{"bash"} + s.langErr = fmt.Errorf("unknown script language %q: supported languages are bash, sh, "+ + "python, python3, node, nodejs, javascript, ruby (use WithScriptImage/WithScriptCommand "+ + "to configure a custom interpreter)", language) } for _, opt := range opts { @@ -107,13 +115,15 @@ func (s *Script) Source(source string) *Script { return s } -// Image overrides the default image for the script. +// Image overrides the default image for the script. Setting an explicit image clears any +// unknown-language error, since the caller is providing the interpreter deliberately. // // Example: // // script.Image("custom/python:3.11") func (s *Script) Image(image string) *Script { s.image = image + s.langErr = nil return s } @@ -204,12 +214,23 @@ func (s *Script) When(condition string) *Script { return s } +// ContinueOn configures the step to continue even when it fails or errors. Without this, +// the continueOn field read by Steps stays nil and the feature is unreachable. +// +// Example: +// +// script.ContinueOn(&v1alpha1.ContinueOn{Failed: true}) +func (s *Script) ContinueOn(continueOn *v1alpha1.ContinueOn) *Script { + s.continueOn = continueOn + return s +} + // WithRetry sets the retry strategy for the script step. // The retry strategy overrides any default retry strategy set on the WorkflowBuilder. // // Example: // -// retryLimit := intstr.FromInt(3) +// retryLimit := intstr.FromInt32(3) // script.WithRetry(&v1alpha1.RetryStrategy{Limit: &retryLimit}) func (s *Script) WithRetry(strategy *v1alpha1.RetryStrategy) *Script { s.retryStrategy = strategy @@ -221,7 +242,7 @@ func (s *Script) Steps() ([]v1alpha1.WorkflowStep, error) { ctx := context.Background() logger := otel.NewLogHelper(ctx, s.otelConfig, - "github.com/jasoet/pkg/v2/argo/builder/template", "Script.Steps") + "github.com/jasoet/pkg/v3/argo/builder/template", "Script.Steps") logger.Debug("Generating script steps", otel.F("name", s.name), otel.F("image", s.image)) @@ -247,17 +268,32 @@ func (s *Script) Templates() ([]v1alpha1.Template, error) { ctx := context.Background() logger := otel.NewLogHelper(ctx, s.otelConfig, - "github.com/jasoet/pkg/v2/argo/builder/template", "Script.Templates") + "github.com/jasoet/pkg/v3/argo/builder/template", "Script.Templates") logger.Debug("Generating script template", otel.F("name", s.templateName), otel.F("image", s.image)) + // Surface an unknown-language error from NewScript rather than silently running the + // source under the wrong interpreter. + if s.langErr != nil { + logger.Error(s.langErr, "Unknown script language") + return nil, s.langErr + } + // Use s.source if set (e.g. from artifact/configmap reference), otherwise fall back to inline scriptContent. source := s.scriptContent if s.source != "" { source = s.source } + // A script with no source (neither inline content nor an artifact/configmap reference) + // builds fine here but is rejected server-side; fail early with a clear message. + if source == "" { + err := fmt.Errorf("script %q has empty source: set inline content (WithScriptContent) or a source reference (Source)", s.name) + logger.Error(err, "Empty script source") + return nil, err + } + script := &v1alpha1.ScriptTemplate{ Container: corev1.Container{ Name: s.name, @@ -302,10 +338,12 @@ func WithScriptContent(content string) ScriptOption { } } -// WithScriptImage sets the container image. +// WithScriptImage sets the container image. Providing an explicit image clears any +// unknown-language error recorded by NewScript. func WithScriptImage(image string) ScriptOption { return func(s *Script) { s.image = image + s.langErr = nil } } @@ -340,6 +378,13 @@ func WithScriptWorkingDir(dir string) ScriptOption { } } +// WithScriptContinueOn configures the step to continue on failure/error. +func WithScriptContinueOn(continueOn *v1alpha1.ContinueOn) ScriptOption { + return func(s *Script) { + s.continueOn = continueOn + } +} + // buildResourceRequirements is a helper to build resource requirements. func buildResourceRequirements(cpuReq, cpuLim, memReq, memLim string) (corev1.ResourceRequirements, error) { reqs := corev1.ResourceRequirements{ diff --git a/argo/builder/template/script_test.go b/argo/builder/template/script_test.go index 4406822..2ef5916 100644 --- a/argo/builder/template/script_test.go +++ b/argo/builder/template/script_test.go @@ -275,6 +275,60 @@ func TestScript_InvalidMemoryQuantity(t *testing.T) { assert.Error(t, err) } +func TestScript_UnknownLanguageErrors(t *testing.T) { + // An unknown language must surface an error from Templates rather than silently + // defaulting to bash and running the source under the wrong interpreter. + script := NewScript("bad-lang", "golang", + WithScriptContent("package main")) + _, err := script.Templates() + require.Error(t, err) + assert.Contains(t, err.Error(), "unknown script language") +} + +func TestScript_UnknownLanguageClearedByExplicitImage(t *testing.T) { + // Providing an explicit image is a deliberate interpreter override and clears the + // unknown-language error. + script := NewScript("custom-lang", "golang", + WithScriptImage("golang:1.25"), + WithScriptCommand("go", "run"), + WithScriptContent("package main; func main() {}")) + templates, err := script.Templates() + require.NoError(t, err) + require.Len(t, templates, 1) + assert.Equal(t, "golang:1.25", templates[0].Script.Image) +} + +func TestScript_EmptySourceErrors(t *testing.T) { + // A script with neither inline content nor a source reference is rejected server-side; + // fail early with a clear message. + script := NewScript("empty", "bash") + _, err := script.Templates() + require.Error(t, err) + assert.Contains(t, err.Error(), "empty source") +} + +func TestScript_ContinueOn(t *testing.T) { + t.Run("method", func(t *testing.T) { + script := NewScript("s", "bash", WithScriptContent("echo hi")). + ContinueOn(&v1alpha1.ContinueOn{Failed: true}) + steps, err := script.Steps() + require.NoError(t, err) + require.Len(t, steps, 1) + require.NotNil(t, steps[0].ContinueOn) + assert.True(t, steps[0].ContinueOn.Failed) + }) + + t.Run("option", func(t *testing.T) { + script := NewScript("s", "bash", + WithScriptContent("echo hi"), + WithScriptContinueOn(&v1alpha1.ContinueOn{Failed: true})) + steps, err := script.Steps() + require.NoError(t, err) + require.NotNil(t, steps[0].ContinueOn) + assert.True(t, steps[0].ContinueOn.Failed) + }) +} + func TestScriptSource(t *testing.T) { t.Run("sets script source from artifact", func(t *testing.T) { script := NewScript("artifact-test", "bash"). diff --git a/argo/client.go b/argo/client.go index 6c354e3..618d442 100644 --- a/argo/client.go +++ b/argo/client.go @@ -2,17 +2,22 @@ package argo import ( "context" + "errors" "fmt" "os" + "strings" "github.com/argoproj/argo-workflows/v3/pkg/apiclient" "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" clientcmdapi "k8s.io/client-go/tools/clientcmd/api" - "github.com/jasoet/pkg/v2/otel" + "github.com/jasoet/pkg/v3/otel" ) +// ErrNilConfig is returned by NewClient when the provided *Config is nil. +var ErrNilConfig = errors.New("argo: config must not be nil") + // NewClient creates a new Argo Workflows client from the given configuration. // It returns the updated context and client, or an error if the connection fails. // @@ -26,7 +31,6 @@ import ( // if err != nil { // return err // } -// defer client.Close() // // Example (in-cluster): // @@ -36,8 +40,18 @@ import ( // // cfg := argo.ServerConfig("https://argo-server:2746", "Bearer token") // ctx, client, err := argo.NewClient(ctx, cfg) +// +// When config.OTelConfig is set, the returned context carries it (via +// otel.ContextWithConfig), so package operations resolve instrumentation +// automatically through otel.ConfigFromContext. func NewClient(ctx context.Context, config *Config) (context.Context, apiclient.Client, error) { - logger := otel.NewLogHelper(ctx, config.OTelConfig, "github.com/jasoet/pkg/v2/argo", "argo.NewClient") + // Guard against a nil config before dereferencing any of its fields, which would + // otherwise panic. + if config == nil { + return nil, nil, ErrNilConfig + } + + logger := otel.NewLogHelper(ctx, config.OTelConfig, "github.com/jasoet/pkg/v3/argo", "argo.NewClient") logger.Debug("Creating Argo Workflows client", otel.F("inCluster", config.InCluster), @@ -81,6 +95,13 @@ func NewClient(ctx context.Context, config *Config) (context.Context, apiclient. } logger.Debug("Successfully created Argo Workflows client") + + // Propagate the configured OTel config through the returned context so + // operations can resolve it via otel.ConfigFromContext. + if config.OTelConfig != nil { + ctx = otel.ContextWithConfig(ctx, config.OTelConfig) + } + return ctx, client, nil } @@ -97,9 +118,7 @@ func NewClient(ctx context.Context, config *Config) (context.Context, apiclient. func NewClientWithOptions(ctx context.Context, opts ...Option) (context.Context, apiclient.Client, error) { config := DefaultConfig() for _, opt := range opts { - if err := opt(config); err != nil { - return nil, nil, fmt.Errorf("failed to apply option: %w", err) - } + opt(config) } return NewClient(ctx, config) } @@ -112,7 +131,7 @@ func NewClientWithOptions(ctx context.Context, opts ...Option) (context.Context, func buildClientConfig(config *Config) clientcmd.ClientConfig { // Note: context.Background() used here since we don't have access to the actual context // This is acceptable as buildClientConfig is called from within NewClient which has the context - logger := otel.NewLogHelper(context.Background(), config.OTelConfig, "github.com/jasoet/pkg/v2/argo", "argo.buildClientConfig") + logger := otel.NewLogHelper(context.Background(), config.OTelConfig, "github.com/jasoet/pkg/v3/argo", "argo.buildClientConfig") // For in-cluster mode, use in-cluster config if config.InCluster { @@ -145,15 +164,22 @@ func buildClientConfig(config *Config) clientcmd.ClientConfig { ) } +// serviceAccountNamespaceFile is the standard location of the namespace file +// mounted into Kubernetes pods. +const serviceAccountNamespaceFile = "/var/run/secrets/kubernetes.io/serviceaccount/namespace" + // inClusterClientConfig implements clientcmd.ClientConfig for in-cluster usage. -type inClusterClientConfig struct{} +type inClusterClientConfig struct { + // namespaceFile overrides the service account namespace file location (test hook). + namespaceFile string +} func (c *inClusterClientConfig) RawConfig() (clientcmdapi.Config, error) { return clientcmdapi.Config{}, fmt.Errorf("RawConfig not supported for in-cluster config") } func (c *inClusterClientConfig) ClientConfig() (*rest.Config, error) { - logger := otel.NewLogHelper(context.Background(), nil, "github.com/jasoet/pkg/v2/argo", "inClusterClientConfig.ClientConfig") + logger := otel.NewLogHelper(context.Background(), nil, "github.com/jasoet/pkg/v3/argo", "inClusterClientConfig.ClientConfig") logger.Debug("Loading in-cluster config") config, err := rest.InClusterConfig() @@ -168,11 +194,15 @@ func (c *inClusterClientConfig) ClientConfig() (*rest.Config, error) { func (c *inClusterClientConfig) Namespace() (string, bool, error) { // Read namespace from the same location that Kubernetes uses - namespaceBytes, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace") + path := c.namespaceFile + if path == "" { + path = serviceAccountNamespaceFile + } + namespaceBytes, err := os.ReadFile(path) if err != nil { return "default", false, err } - return string(namespaceBytes), true, nil + return strings.TrimSpace(string(namespaceBytes)), true, nil } func (c *inClusterClientConfig) ConfigAccess() clientcmd.ConfigAccess { diff --git a/argo/client_test.go b/argo/client_test.go index 3c8ac56..e368d22 100644 --- a/argo/client_test.go +++ b/argo/client_test.go @@ -6,6 +6,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/jasoet/pkg/v3/otel" ) func TestNewClientWithOptions_AppliesOptions(t *testing.T) { @@ -126,3 +128,30 @@ func TestNewClient_OptionErrors(t *testing.T) { require.Error(t, err) }) } + +func TestNewClientWithOptions_InjectsOTelConfigIntoContext(t *testing.T) { + otelCfg := otel.NewConfig("test") + + t.Run("with OTel config", func(t *testing.T) { + ctx, client, err := NewClientWithOptions(context.Background(), + WithArgoServer("http://nonexistent:2746", "Bearer token"), + WithArgoServerInsecure(true), + WithOTelConfig(otelCfg), + ) + require.NoError(t, err) + require.NotNil(t, client) + assert.Same(t, otelCfg, otel.ConfigFromContext(ctx), + "returned ctx should carry the configured OTelConfig") + }) + + t.Run("without OTel config", func(t *testing.T) { + ctx, client, err := NewClientWithOptions(context.Background(), + WithArgoServer("http://nonexistent:2746", "Bearer token"), + WithArgoServerInsecure(true), + ) + require.NoError(t, err) + require.NotNil(t, client) + assert.Nil(t, otel.ConfigFromContext(ctx), + "returned ctx should not carry an OTelConfig when none is configured") + }) +} diff --git a/argo/client_unit_test.go b/argo/client_unit_test.go new file mode 100644 index 0000000..ff3a53d --- /dev/null +++ b/argo/client_unit_test.go @@ -0,0 +1,47 @@ +package argo + +import ( + "context" + "os" + "path/filepath" + "reflect" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/jasoet/pkg/v3/otel" +) + +func TestNewClient_NilConfigReturnsError(t *testing.T) { + // NewClient must return an error (not panic) when handed a nil config. + ctx, client, err := NewClient(context.Background(), nil) + require.Error(t, err) + assert.ErrorIs(t, err, ErrNilConfig) + assert.Nil(t, ctx) + assert.Nil(t, client) +} + +func TestNamespaceTrimsNewline(t *testing.T) { + namespaceFile := filepath.Join(t.TempDir(), "namespace") + require.NoError(t, os.WriteFile(namespaceFile, []byte("production\n"), 0o600)) + + icc := &inClusterClientConfig{namespaceFile: namespaceFile} + + namespace, overridden, err := icc.Namespace() + + require.NoError(t, err) + assert.True(t, overridden) + assert.Equal(t, "production", namespace, "namespace must be trimmed of the trailing newline") +} + +// TestArchtestArgo mirrors the internal/archtest registry check for the argo +// package: Config must carry OTelConfig *otel.Config tagged `yaml:"-" mapstructure:"-"`. +func TestArchtestArgo(t *testing.T) { + field, ok := reflect.TypeOf(Config{}).FieldByName("OTelConfig") + require.True(t, ok, "argo.Config: missing OTelConfig field") + + assert.Equal(t, reflect.TypeOf(&otel.Config{}), field.Type, "OTelConfig must be *otel.Config") + assert.Equal(t, "-", field.Tag.Get("yaml"), "OTelConfig yaml tag") + assert.Equal(t, "-", field.Tag.Get("mapstructure"), "OTelConfig mapstructure tag") +} diff --git a/argo/config.go b/argo/config.go index f05f3a8..0fa7bc5 100644 --- a/argo/config.go +++ b/argo/config.go @@ -1,7 +1,7 @@ package argo import ( - "github.com/jasoet/pkg/v2/otel" + "github.com/jasoet/pkg/v3/otel" ) // Config represents the configuration for connecting to Argo Workflows. @@ -28,7 +28,7 @@ type Config struct { ArgoServerOpts ServerOpts `yaml:"argoServer" mapstructure:"argoServer"` // OTelConfig enables OpenTelemetry instrumentation (optional). - OTelConfig *otel.Config `yaml:"-"` + OTelConfig *otel.Config `yaml:"-" mapstructure:"-"` } // ServerOpts contains Argo Server connection options. diff --git a/argo/example_test.go b/argo/example_test.go new file mode 100644 index 0000000..e77c459 --- /dev/null +++ b/argo/example_test.go @@ -0,0 +1,74 @@ +package argo_test + +import ( + "context" + "fmt" + + "github.com/jasoet/pkg/v3/argo" + "github.com/jasoet/pkg/v3/argo/builder" + "github.com/jasoet/pkg/v3/argo/builder/template" + "github.com/jasoet/pkg/v3/otel" +) + +// ExampleNewClientWithOptions demonstrates creating an Argo Workflows client +// with functional options. The returned context carries the configured OTel +// config, so package operations called with that context are instrumented +// automatically. +// +// This example requires a reachable Kubernetes cluster and does not run +// during `go test` (no Output comment). +func ExampleNewClientWithOptions() { + ctx := context.Background() + + otelConfig := otel.NewConfig("my-service") + + // The returned ctx carries the OTel config configured via WithOTelConfig. + ctx, client, err := argo.NewClientWithOptions(ctx, + argo.WithKubeConfig("/path/to/kubeconfig"), + argo.WithContext("production"), + argo.WithOTelConfig(otelConfig), + ) + if err != nil { + fmt.Println("failed to create client:", err) + return + } + + fmt.Println("client created:", client != nil, "ctx:", ctx != nil) +} + +// ExampleSubmitWorkflow demonstrates building a workflow with the fluent +// builder API and submitting it. SubmitWorkflow resolves its OpenTelemetry +// config from the context via otel.ConfigFromContext — when ctx came from a +// client created with argo.WithOTelConfig, the call is instrumented without +// any extra argument. +// +// This example requires a reachable Argo Workflows installation and does not +// run during `go test` (no Output comment). +func ExampleSubmitWorkflow() { + ctx := context.Background() + + ctx, client, err := argo.NewClient(ctx, argo.DefaultConfig()) + if err != nil { + fmt.Println("failed to create client:", err) + return + } + + hello := template.NewContainer("hello", "alpine:latest", + template.WithCommand("echo", "Hello, Argo!")) + + wf, err := builder.NewWorkflowBuilder("hello-world", "argo"). + Add(hello). + Build() + if err != nil { + fmt.Println("failed to build workflow:", err) + return + } + + created, err := argo.SubmitWorkflow(ctx, client, wf) + if err != nil { + fmt.Println("failed to submit workflow:", err) + return + } + + fmt.Println("submitted workflow:", created.Name) +} diff --git a/argo/operations.go b/argo/operations.go index 7ff641a..d662fcb 100644 --- a/argo/operations.go +++ b/argo/operations.go @@ -2,6 +2,7 @@ package argo import ( "context" + "errors" "fmt" "time" @@ -10,11 +11,72 @@ import ( "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/trace" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "github.com/jasoet/pkg/v2/otel" + "github.com/jasoet/pkg/v3/otel" ) +// Sentinel errors returned by workflow operations. Callers can match them with errors.Is. +var ( + // ErrWorkflowFailed is returned by SubmitAndWait when the workflow reaches a terminal + // Failed or Error phase. + ErrWorkflowFailed = errors.New("argo: workflow failed") + + // ErrWaitTimeout is returned by SubmitAndWait when the wait deadline elapses before the + // workflow completes. The returned error also wraps context.DeadlineExceeded. + ErrWaitTimeout = errors.New("argo: timed out waiting for workflow to complete") +) + +// defaultPollInterval is how often SubmitAndWait polls the workflow status when no +// WithPollInterval option is supplied. +const defaultPollInterval = 5 * time.Second + +// waitOptions holds tunables for SubmitAndWait. +type waitOptions struct { + pollInterval time.Duration +} + +// WaitOption configures SubmitAndWait. +type WaitOption func(*waitOptions) + +// WithPollInterval overrides how often SubmitAndWait polls the workflow status. +// Non-positive values are ignored (the default interval is used). +func WithPollInterval(d time.Duration) WaitOption { + return func(o *waitOptions) { + if d > 0 { + o.pollInterval = d + } + } +} + +// isTransientPollError reports whether a GetWorkflow polling error is transient (worth +// retrying) rather than permanent. Permanent errors — the workflow does not exist, or the +// caller lacks permission — would otherwise spin uselessly until the wait deadline. +func isTransientPollError(err error) bool { + if err == nil { + return false + } + // A per-call deadline/cancellation is handled by the outer select, but be defensive. + if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) { + return true + } + st, ok := status.FromError(err) + if !ok { + // Not a gRPC status error: treat as transient so genuinely flaky/network errors + // keep retrying until the deadline. + return true + } + switch st.Code() { + case codes.NotFound, codes.PermissionDenied, codes.Unauthenticated, + codes.InvalidArgument, codes.FailedPrecondition, codes.Unimplemented: + return false + default: + return true + } +} + // SubmitWorkflow submits a workflow to Argo with OpenTelemetry tracing. // This is a convenience wrapper around the Argo API client with better error handling // and automatic observability. @@ -28,21 +90,23 @@ import ( // return err // } // -// created, err := argo.SubmitWorkflow(ctx, client, wf, otelConfig) +// created, err := argo.SubmitWorkflow(ctx, client, wf) // if err != nil { // return err // } // fmt.Printf("Workflow %s submitted\n", created.Name) -func SubmitWorkflow(ctx context.Context, client apiclient.Client, wf *v1alpha1.Workflow, cfg *otel.Config) (*v1alpha1.Workflow, error) { +func SubmitWorkflow(ctx context.Context, client apiclient.Client, wf *v1alpha1.Workflow) (*v1alpha1.Workflow, error) { + cfg := otel.ConfigFromContext(ctx) + // Start span var span trace.Span if cfg != nil && cfg.TracerProvider != nil { - tracer := cfg.TracerProvider.Tracer("github.com/jasoet/pkg/v2/argo") + tracer := cfg.TracerProvider.Tracer("github.com/jasoet/pkg/v3/argo") ctx, span = tracer.Start(ctx, "argo.SubmitWorkflow") defer span.End() } - logger := otel.NewLogHelper(ctx, cfg, "github.com/jasoet/pkg/v2/argo", "argo.SubmitWorkflow") + logger := otel.NewLogHelper(ctx, cfg, "github.com/jasoet/pkg/v3/argo", "argo.SubmitWorkflow") logger.Info("Submitting workflow", otel.F("workflow_generate_name", wf.GenerateName), otel.F("namespace", wf.Namespace)) @@ -87,103 +151,145 @@ func SubmitWorkflow(ctx context.Context, client apiclient.Client, wf *v1alpha1.W // return err // } // -// completed, err := argo.SubmitAndWait(ctx, client, wf, otelConfig, 10*time.Minute) +// completed, err := argo.SubmitAndWait(ctx, client, wf, 10*time.Minute) // if err != nil { // return err // } // if completed.Status.Phase == v1alpha1.WorkflowSucceeded { // fmt.Println("Workflow completed successfully") // } -func SubmitAndWait(ctx context.Context, client apiclient.Client, wf *v1alpha1.Workflow, cfg *otel.Config, timeout time.Duration) (*v1alpha1.Workflow, error) { +func SubmitAndWait(ctx context.Context, client apiclient.Client, wf *v1alpha1.Workflow, timeout time.Duration, opts ...WaitOption) (*v1alpha1.Workflow, error) { + cfg := otel.ConfigFromContext(ctx) + + options := waitOptions{pollInterval: defaultPollInterval} + for _, opt := range opts { + opt(&options) + } + // Start span for entire operation var span trace.Span if cfg != nil && cfg.TracerProvider != nil { - tracer := cfg.TracerProvider.Tracer("github.com/jasoet/pkg/v2/argo") + tracer := cfg.TracerProvider.Tracer("github.com/jasoet/pkg/v3/argo") ctx, span = tracer.Start(ctx, "argo.SubmitAndWait") defer span.End() } - logger := otel.NewLogHelper(ctx, cfg, "github.com/jasoet/pkg/v2/argo", "argo.SubmitAndWait") + logger := otel.NewLogHelper(ctx, cfg, "github.com/jasoet/pkg/v3/argo", "argo.SubmitAndWait") startTime := time.Now() // Submit workflow - created, err := SubmitWorkflow(ctx, client, wf, cfg) + created, err := SubmitWorkflow(ctx, client, wf) if err != nil { return nil, err } logger.Info("Waiting for workflow completion", otel.F("workflow_name", created.Name), - otel.F("timeout", timeout.String())) - - // Wait for completion with polling - ticker := time.NewTicker(5 * time.Second) - defer ticker.Stop() + otel.F("timeout", timeout.String()), + otel.F("poll_interval", options.pollInterval.String())) timeoutCtx, cancel := context.WithTimeout(ctx, timeout) defer cancel() wfClient := client.NewWorkflowServiceClient() - for { - select { - case <-timeoutCtx.Done(): - err := fmt.Errorf("timeout waiting for workflow: %s", created.Name) - logger.Error(err, "Workflow timed out", + // poll performs a single status check. It returns (workflowToReturn, terminal, err): + // - terminal true with err nil => workflow succeeded + // - err non-nil => terminal failure or permanent poll error + // - terminal false, err nil => still running (or a transient poll error to retry) + poll := func() (*v1alpha1.Workflow, bool, error) { + result, gErr := wfClient.GetWorkflow(timeoutCtx, &workflow.WorkflowGetRequest{ + Namespace: created.Namespace, + Name: created.Name, + }) + if gErr != nil { + if isTransientPollError(gErr) { + logger.Warn("Transient error getting workflow status, will retry", + otel.F("workflow_name", created.Name), + otel.F("error", gErr.Error())) + return nil, false, nil + } + // Permanent error (e.g. NotFound/PermissionDenied): abort instead of spinning + // until the deadline. + wErr := fmt.Errorf("failed to get workflow status for %q: %w", created.Name, gErr) + logger.Error(wErr, "Permanent error getting workflow status; aborting wait", + otel.F("workflow_name", created.Name)) + return created, false, wErr + } + + switch result.Status.Phase { + case v1alpha1.WorkflowSucceeded: + duration := time.Since(startTime) + logger.Info("Workflow succeeded", otel.F("workflow_name", created.Name), - otel.F("duration", time.Since(startTime).String())) - return created, err + otel.F("duration", duration.String())) + if span != nil && span.IsRecording() { + span.SetAttributes( + attribute.String("workflow.status", "succeeded"), + attribute.Float64("workflow.duration_seconds", duration.Seconds()), + ) + } + return result, true, nil - case <-ticker.C: - result, err := wfClient.GetWorkflow(timeoutCtx, &workflow.WorkflowGetRequest{ - Namespace: created.Namespace, - Name: created.Name, - }) - if err != nil { - logger.Warn("Failed to get workflow status", otel.F("error", err.Error())) - continue + case v1alpha1.WorkflowFailed, v1alpha1.WorkflowError: + duration := time.Since(startTime) + wErr := fmt.Errorf("%w: %q (phase: %s): %s", ErrWorkflowFailed, created.Name, result.Status.Phase, result.Status.Message) + logger.Error(wErr, "Workflow failed", + otel.F("workflow_name", created.Name), + otel.F("phase", string(result.Status.Phase)), + otel.F("duration", duration.String())) + if span != nil && span.IsRecording() { + span.SetAttributes( + attribute.String("workflow.status", "failed"), + attribute.String("workflow.phase", string(result.Status.Phase)), + attribute.Float64("workflow.duration_seconds", duration.Seconds()), + ) } + return result, true, wErr - // Check if workflow is complete - if result.Status.Phase == v1alpha1.WorkflowSucceeded { - duration := time.Since(startTime) - logger.Info("Workflow succeeded", - otel.F("workflow_name", created.Name), - otel.F("duration", duration.String())) + default: + logger.Debug("Workflow still running", + otel.F("workflow_name", created.Name), + otel.F("phase", string(result.Status.Phase))) + return result, false, nil + } + } - if span != nil && span.IsRecording() { - span.SetAttributes( - attribute.String("workflow.status", "succeeded"), - attribute.Float64("workflow.duration_seconds", duration.Seconds()), - ) - } + // Poll immediately so a workflow that is already terminal is detected without waiting a + // full interval. + if result, terminal, pErr := poll(); terminal || pErr != nil { + return result, pErr + } - return result, nil - } + ticker := time.NewTicker(options.pollInterval) + defer ticker.Stop() - if result.Status.Phase == v1alpha1.WorkflowFailed || result.Status.Phase == v1alpha1.WorkflowError { - duration := time.Since(startTime) - err := fmt.Errorf("workflow failed with phase: %s, message: %s", result.Status.Phase, result.Status.Message) - logger.Error(err, "Workflow failed", + for { + select { + case <-timeoutCtx.Done(): + cause := timeoutCtx.Err() + duration := time.Since(startTime) + if errors.Is(cause, context.DeadlineExceeded) { + // Wrap both the sentinel and context.DeadlineExceeded so callers can match + // either with errors.Is. + wErr := fmt.Errorf("%w: %q after %s: %w", ErrWaitTimeout, created.Name, duration, cause) + logger.Error(wErr, "Workflow wait timed out", otel.F("workflow_name", created.Name), - otel.F("phase", string(result.Status.Phase)), otel.F("duration", duration.String())) - - if span != nil && span.IsRecording() { - span.SetAttributes( - attribute.String("workflow.status", "failed"), - attribute.String("workflow.phase", string(result.Status.Phase)), - attribute.Float64("workflow.duration_seconds", duration.Seconds()), - ) - } - - return result, err + return created, wErr } - - logger.Debug("Workflow still running", + // Parent context canceled (not a timeout): label it accurately. + wErr := fmt.Errorf("waiting for workflow %q canceled after %s: %w", created.Name, duration, cause) + logger.Error(wErr, "Workflow wait canceled", otel.F("workflow_name", created.Name), - otel.F("phase", string(result.Status.Phase))) + otel.F("duration", duration.String())) + return created, wErr + + case <-ticker.C: + if result, terminal, pErr := poll(); terminal || pErr != nil { + return result, pErr + } } } } @@ -192,13 +298,15 @@ func SubmitAndWait(ctx context.Context, client apiclient.Client, wf *v1alpha1.Wo // // Example: // -// status, err := argo.GetWorkflowStatus(ctx, client, "argo", "my-workflow-abc123", otelConfig) +// status, err := argo.GetWorkflowStatus(ctx, client, "argo", "my-workflow-abc123") // if err != nil { // return err // } // fmt.Printf("Workflow phase: %s\n", status.Phase) -func GetWorkflowStatus(ctx context.Context, client apiclient.Client, namespace, name string, cfg *otel.Config) (*v1alpha1.WorkflowStatus, error) { - logger := otel.NewLogHelper(ctx, cfg, "github.com/jasoet/pkg/v2/argo", "argo.GetWorkflowStatus") +func GetWorkflowStatus(ctx context.Context, client apiclient.Client, namespace, name string) (*v1alpha1.WorkflowStatus, error) { + cfg := otel.ConfigFromContext(ctx) + + logger := otel.NewLogHelper(ctx, cfg, "github.com/jasoet/pkg/v3/argo", "argo.GetWorkflowStatus") logger.Debug("Getting workflow status", otel.F("namespace", namespace), otel.F("name", name)) @@ -227,50 +335,67 @@ func GetWorkflowStatus(ctx context.Context, client apiclient.Client, namespace, // Example: // // // List all workflows -// workflows, err := argo.ListWorkflows(ctx, client, "argo", "", otelConfig) +// workflows, err := argo.ListWorkflows(ctx, client, "argo", "") // // // List workflows with label -// workflows, err := argo.ListWorkflows(ctx, client, "argo", "app=myapp", otelConfig) -func ListWorkflows(ctx context.Context, client apiclient.Client, namespace, labelSelector string, cfg *otel.Config) ([]v1alpha1.Workflow, error) { - logger := otel.NewLogHelper(ctx, cfg, "github.com/jasoet/pkg/v2/argo", "argo.ListWorkflows") +// workflows, err := argo.ListWorkflows(ctx, client, "argo", "app=myapp") +func ListWorkflows(ctx context.Context, client apiclient.Client, namespace, labelSelector string) ([]v1alpha1.Workflow, error) { + cfg := otel.ConfigFromContext(ctx) + + logger := otel.NewLogHelper(ctx, cfg, "github.com/jasoet/pkg/v3/argo", "argo.ListWorkflows") logger.Debug("Listing workflows", otel.F("namespace", namespace), otel.F("label_selector", labelSelector)) wfClient := client.NewWorkflowServiceClient() - listOpts := &metav1.ListOptions{} - if labelSelector != "" { - listOpts.LabelSelector = labelSelector - } + // Follow pagination continue tokens so callers get the full result set rather than a + // truncated first page. + var all []v1alpha1.Workflow + continueToken := "" + for { + listOpts := &metav1.ListOptions{Continue: continueToken} + if labelSelector != "" { + listOpts.LabelSelector = labelSelector + } - resp, err := wfClient.ListWorkflows(ctx, &workflow.WorkflowListRequest{ - Namespace: namespace, - ListOptions: listOpts, - }) - if err != nil { - logger.Error(err, "Failed to list workflows", - otel.F("namespace", namespace)) - return nil, fmt.Errorf("failed to list workflows: %w", err) + resp, err := wfClient.ListWorkflows(ctx, &workflow.WorkflowListRequest{ + Namespace: namespace, + ListOptions: listOpts, + }) + if err != nil { + logger.Error(err, "Failed to list workflows", + otel.F("namespace", namespace)) + return nil, fmt.Errorf("failed to list workflows: %w", err) + } + + all = append(all, resp.Items...) + + continueToken = resp.Continue + if continueToken == "" { + break + } } logger.Info("Listed workflows", otel.F("namespace", namespace), - otel.F("count", len(resp.Items))) + otel.F("count", len(all))) - return resp.Items, nil + return all, nil } // DeleteWorkflow deletes a workflow by name. // // Example: // -// err := argo.DeleteWorkflow(ctx, client, "argo", "my-workflow-abc123", otelConfig) +// err := argo.DeleteWorkflow(ctx, client, "argo", "my-workflow-abc123") // if err != nil { // return err // } -func DeleteWorkflow(ctx context.Context, client apiclient.Client, namespace, name string, cfg *otel.Config) error { - logger := otel.NewLogHelper(ctx, cfg, "github.com/jasoet/pkg/v2/argo", "argo.DeleteWorkflow") +func DeleteWorkflow(ctx context.Context, client apiclient.Client, namespace, name string) error { + cfg := otel.ConfigFromContext(ctx) + + logger := otel.NewLogHelper(ctx, cfg, "github.com/jasoet/pkg/v3/argo", "argo.DeleteWorkflow") logger.Info("Deleting workflow", otel.F("namespace", namespace), otel.F("name", name)) diff --git a/argo/operations_integration_test.go b/argo/operations_integration_test.go index b884e32..ff3d0b6 100644 --- a/argo/operations_integration_test.go +++ b/argo/operations_integration_test.go @@ -13,9 +13,9 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/intstr" - "github.com/jasoet/pkg/v2/argo/builder" - "github.com/jasoet/pkg/v2/argo/builder/template" - "github.com/jasoet/pkg/v2/otel" + "github.com/jasoet/pkg/v3/argo/builder" + "github.com/jasoet/pkg/v3/argo/builder/template" + "github.com/jasoet/pkg/v3/otel" ) func TestIntegration_SubmitWorkflow(t *testing.T) { @@ -36,7 +36,7 @@ func TestIntegration_SubmitWorkflow(t *testing.T) { require.NoError(t, err, "should build workflow") // Submit workflow - created, err := SubmitWorkflow(ctx, client, wf, cfg) + created, err := SubmitWorkflow(ctx, client, wf) require.NoError(t, err, "should submit workflow") require.NotNil(t, created) assert.NotEmpty(t, created.Name) @@ -44,7 +44,7 @@ func TestIntegration_SubmitWorkflow(t *testing.T) { // Cleanup defer func() { - _ = DeleteWorkflow(ctx, client, "argo", created.Name, cfg) + _ = DeleteWorkflow(ctx, client, "argo", created.Name) }() t.Logf("✓ Workflow submitted: %s", created.Name) @@ -68,7 +68,7 @@ func TestIntegration_SubmitAndWait(t *testing.T) { require.NoError(t, err, "should build workflow") // Submit and wait - completed, err := SubmitAndWait(ctx, client, wf, cfg, 2*time.Minute) + completed, err := SubmitAndWait(ctx, client, wf, 2*time.Minute) require.NoError(t, err, "should complete workflow") require.NotNil(t, completed) assert.Contains(t, []v1alpha1.WorkflowPhase{ @@ -78,7 +78,7 @@ func TestIntegration_SubmitAndWait(t *testing.T) { // Cleanup defer func() { - _ = DeleteWorkflow(ctx, client, "argo", completed.Name, cfg) + _ = DeleteWorkflow(ctx, client, "argo", completed.Name) }() t.Logf("✓ Workflow completed: %s (phase: %s)", completed.Name, completed.Status.Phase) @@ -101,12 +101,12 @@ func TestIntegration_GetWorkflowStatus(t *testing.T) { Build() require.NoError(t, err) - created, err := SubmitWorkflow(ctx, client, wf, cfg) + created, err := SubmitWorkflow(ctx, client, wf) require.NoError(t, err) // Cleanup defer func() { - _ = DeleteWorkflow(ctx, client, "argo", created.Name, cfg) + _ = DeleteWorkflow(ctx, client, "argo", created.Name) }() // Wait a moment for workflow to initialize. @@ -115,7 +115,7 @@ func TestIntegration_GetWorkflowStatus(t *testing.T) { time.Sleep(2 * time.Second) // Get workflow status - status, err := GetWorkflowStatus(ctx, client, "argo", created.Name, cfg) + status, err := GetWorkflowStatus(ctx, client, "argo", created.Name) require.NoError(t, err, "should get workflow status") require.NotNil(t, status) assert.NotEmpty(t, status.Phase) @@ -143,21 +143,21 @@ func TestIntegration_ListWorkflows(t *testing.T) { Build() require.NoError(t, err) - created, err := SubmitWorkflow(ctx, client, wf, cfg) + created, err := SubmitWorkflow(ctx, client, wf) require.NoError(t, err) // Cleanup defer func() { - _ = DeleteWorkflow(ctx, client, "argo", created.Name, cfg) + _ = DeleteWorkflow(ctx, client, "argo", created.Name) }() // List all workflows - workflows, err := ListWorkflows(ctx, client, "argo", "", cfg) + workflows, err := ListWorkflows(ctx, client, "argo", "") require.NoError(t, err, "should list workflows") assert.NotEmpty(t, workflows, "should have at least one workflow") // List with label selector - filtered, err := ListWorkflows(ctx, client, "argo", "test-type=integration-list", cfg) + filtered, err := ListWorkflows(ctx, client, "argo", "test-type=integration-list") require.NoError(t, err, "should list filtered workflows") assert.NotEmpty(t, filtered, "should find workflow with label") @@ -191,7 +191,7 @@ func TestIntegration_DeleteWorkflow(t *testing.T) { Build() require.NoError(t, err) - created, err := SubmitWorkflow(ctx, client, wf, cfg) + created, err := SubmitWorkflow(ctx, client, wf) require.NoError(t, err) // Wait a moment for workflow to be fully created. @@ -200,13 +200,13 @@ func TestIntegration_DeleteWorkflow(t *testing.T) { time.Sleep(2 * time.Second) // Delete workflow - err = DeleteWorkflow(ctx, client, "argo", created.Name, cfg) + err = DeleteWorkflow(ctx, client, "argo", created.Name) require.NoError(t, err, "should delete workflow") // Verify deletion - getting the workflow should fail. // Note: time.Sleep allows the API server cache to propagate the deletion. time.Sleep(1 * time.Second) - status, err := GetWorkflowStatus(ctx, client, "argo", created.Name, cfg) + status, err := GetWorkflowStatus(ctx, client, "argo", created.Name) if err == nil && status != nil { // Workflow might still exist briefly after deletion t.Logf("Workflow still exists briefly: %s", created.Name) @@ -240,17 +240,17 @@ func TestIntegration_CompleteWorkflow(t *testing.T) { require.NoError(t, err) // Submit and wait for completion - completed, err := SubmitAndWait(ctx, client, wf, cfg, 2*time.Minute) + completed, err := SubmitAndWait(ctx, client, wf, 2*time.Minute) require.NoError(t, err, "should complete workflow") require.NotNil(t, completed) // Cleanup defer func() { - _ = DeleteWorkflow(ctx, client, "argo", completed.Name, cfg) + _ = DeleteWorkflow(ctx, client, "argo", completed.Name) }() // Verify completion - status, err := GetWorkflowStatus(ctx, client, "argo", completed.Name, cfg) + status, err := GetWorkflowStatus(ctx, client, "argo", completed.Name) require.NoError(t, err) assert.Contains(t, []v1alpha1.WorkflowPhase{ v1alpha1.WorkflowSucceeded, @@ -280,13 +280,13 @@ func TestIntegration_WorkflowWithResources(t *testing.T) { require.NoError(t, err) // Submit workflow - created, err := SubmitWorkflow(ctx, client, wf, cfg) + created, err := SubmitWorkflow(ctx, client, wf) require.NoError(t, err) require.NotNil(t, created) // Cleanup defer func() { - _ = DeleteWorkflow(ctx, client, "argo", created.Name, cfg) + _ = DeleteWorkflow(ctx, client, "argo", created.Name) }() // Verify resource limits were set @@ -324,13 +324,13 @@ func TestIntegration_WorkflowWithScript(t *testing.T) { require.NoError(t, err) // Submit workflow - created, err := SubmitWorkflow(ctx, client, wf, cfg) + created, err := SubmitWorkflow(ctx, client, wf) require.NoError(t, err) require.NotNil(t, created) // Cleanup defer func() { - _ = DeleteWorkflow(ctx, client, "argo", created.Name, cfg) + _ = DeleteWorkflow(ctx, client, "argo", created.Name) }() t.Logf("✓ Workflow with script submitted: %s", created.Name) @@ -366,14 +366,14 @@ func TestIntegration_WorkflowWithParameters(t *testing.T) { } // Submit workflow - created, err := SubmitWorkflow(ctx, client, wf, cfg) + created, err := SubmitWorkflow(ctx, client, wf) require.NoError(t, err, "should submit workflow with parameters") require.NotNil(t, created) assert.NotEmpty(t, created.Name) // Cleanup defer func() { - _ = DeleteWorkflow(ctx, client, "argo", created.Name, cfg) + _ = DeleteWorkflow(ctx, client, "argo", created.Name) }() t.Logf("✓ Workflow with parameters submitted: %s", created.Name) @@ -403,7 +403,7 @@ func TestIntegration_WorkflowWithRetryStrategy(t *testing.T) { require.NoError(t, err, "should build workflow with retry") // Submit workflow - created, err := SubmitWorkflow(ctx, client, wf, cfg) + created, err := SubmitWorkflow(ctx, client, wf) require.NoError(t, err, "should submit workflow with retry strategy") require.NotNil(t, created) @@ -412,7 +412,7 @@ func TestIntegration_WorkflowWithRetryStrategy(t *testing.T) { // Cleanup defer func() { - _ = DeleteWorkflow(ctx, client, "argo", created.Name, cfg) + _ = DeleteWorkflow(ctx, client, "argo", created.Name) }() t.Logf("✓ Workflow with retry strategy submitted: %s", created.Name) @@ -446,7 +446,7 @@ func TestIntegration_WorkflowWithVolumes(t *testing.T) { require.NoError(t, err, "should build workflow with volumes") // Submit workflow - created, err := SubmitWorkflow(ctx, client, wf, cfg) + created, err := SubmitWorkflow(ctx, client, wf) require.NoError(t, err, "should submit workflow with volumes") require.NotNil(t, created) @@ -456,7 +456,7 @@ func TestIntegration_WorkflowWithVolumes(t *testing.T) { // Cleanup defer func() { - _ = DeleteWorkflow(ctx, client, "argo", created.Name, cfg) + _ = DeleteWorkflow(ctx, client, "argo", created.Name) }() t.Logf("✓ Workflow with volumes submitted: %s", created.Name) @@ -483,7 +483,7 @@ func TestIntegration_WorkflowWithEnvironmentVariables(t *testing.T) { require.NoError(t, err, "should build workflow with env vars") // Submit workflow - created, err := SubmitWorkflow(ctx, client, wf, cfg) + created, err := SubmitWorkflow(ctx, client, wf) require.NoError(t, err, "should submit workflow with env vars") require.NotNil(t, created) @@ -509,7 +509,7 @@ func TestIntegration_WorkflowWithEnvironmentVariables(t *testing.T) { // Cleanup defer func() { - _ = DeleteWorkflow(ctx, client, "argo", created.Name, cfg) + _ = DeleteWorkflow(ctx, client, "argo", created.Name) }() t.Logf("✓ Workflow with environment variables submitted: %s", created.Name) @@ -538,13 +538,13 @@ func TestIntegration_WorkflowWithConditionalSteps(t *testing.T) { require.NoError(t, err, "should build workflow with conditional") // Submit workflow - created, err := SubmitWorkflow(ctx, client, wf, cfg) + created, err := SubmitWorkflow(ctx, client, wf) require.NoError(t, err, "should submit workflow with conditional") require.NotNil(t, created) // Cleanup defer func() { - _ = DeleteWorkflow(ctx, client, "argo", created.Name, cfg) + _ = DeleteWorkflow(ctx, client, "argo", created.Name) }() t.Logf("✓ Workflow with conditional steps submitted: %s", created.Name) @@ -569,7 +569,7 @@ func TestIntegration_WorkflowWithHTTPTemplate(t *testing.T) { require.NoError(t, err, "should build workflow with HTTP") // Submit workflow - created, err := SubmitWorkflow(ctx, client, wf, cfg) + created, err := SubmitWorkflow(ctx, client, wf) require.NoError(t, err, "should submit workflow with HTTP template") require.NotNil(t, created) @@ -587,7 +587,7 @@ func TestIntegration_WorkflowWithHTTPTemplate(t *testing.T) { // Cleanup defer func() { - _ = DeleteWorkflow(ctx, client, "argo", created.Name, cfg) + _ = DeleteWorkflow(ctx, client, "argo", created.Name) }() t.Logf("✓ Workflow with HTTP template submitted: %s", created.Name) @@ -615,7 +615,7 @@ func TestIntegration_WorkflowWithMultipleContainers(t *testing.T) { require.NoError(t, err, "should build workflow with multiple steps") // Submit workflow - created, err := SubmitWorkflow(ctx, client, wf, cfg) + created, err := SubmitWorkflow(ctx, client, wf) require.NoError(t, err, "should submit workflow with multiple containers") require.NotNil(t, created) @@ -624,7 +624,7 @@ func TestIntegration_WorkflowWithMultipleContainers(t *testing.T) { // Cleanup defer func() { - _ = DeleteWorkflow(ctx, client, "argo", created.Name, cfg) + _ = DeleteWorkflow(ctx, client, "argo", created.Name) }() t.Logf("✓ Workflow with multiple containers submitted: %s", created.Name) @@ -657,7 +657,7 @@ func TestIntegration_WorkflowWithLabelsAndAnnotations(t *testing.T) { require.NoError(t, err, "should build workflow with metadata") // Submit workflow - created, err := SubmitWorkflow(ctx, client, wf, cfg) + created, err := SubmitWorkflow(ctx, client, wf) require.NoError(t, err, "should submit workflow with metadata") require.NotNil(t, created) @@ -670,7 +670,7 @@ func TestIntegration_WorkflowWithLabelsAndAnnotations(t *testing.T) { // Cleanup defer func() { - _ = DeleteWorkflow(ctx, client, "argo", created.Name, cfg) + _ = DeleteWorkflow(ctx, client, "argo", created.Name) }() t.Logf("✓ Workflow with labels and annotations submitted: %s", created.Name) @@ -698,7 +698,7 @@ func TestIntegration_WorkflowWithTTL(t *testing.T) { require.NoError(t, err, "should build workflow with TTL") // Submit workflow - created, err := SubmitWorkflow(ctx, client, wf, cfg) + created, err := SubmitWorkflow(ctx, client, wf) require.NoError(t, err, "should submit workflow with TTL") require.NotNil(t, created) @@ -709,7 +709,7 @@ func TestIntegration_WorkflowWithTTL(t *testing.T) { // Cleanup defer func() { - _ = DeleteWorkflow(ctx, client, "argo", created.Name, cfg) + _ = DeleteWorkflow(ctx, client, "argo", created.Name) }() t.Logf("✓ Workflow with TTL submitted: %s", created.Name) @@ -735,7 +735,7 @@ func TestIntegration_WorkflowWithArchiveLogs(t *testing.T) { require.NoError(t, err, "should build workflow with archive logs") // Submit workflow - created, err := SubmitWorkflow(ctx, client, wf, cfg) + created, err := SubmitWorkflow(ctx, client, wf) require.NoError(t, err, "should submit workflow with archive logs") require.NotNil(t, created) @@ -745,7 +745,7 @@ func TestIntegration_WorkflowWithArchiveLogs(t *testing.T) { // Cleanup defer func() { - _ = DeleteWorkflow(ctx, client, "argo", created.Name, cfg) + _ = DeleteWorkflow(ctx, client, "argo", created.Name) }() t.Logf("✓ Workflow with archive logs submitted: %s", created.Name) diff --git a/argo/operations_test.go b/argo/operations_test.go index a79a30a..698c952 100644 --- a/argo/operations_test.go +++ b/argo/operations_test.go @@ -15,10 +15,14 @@ import ( "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + sdktrace "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/sdk/trace/tracetest" "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "github.com/jasoet/pkg/v2/otel" + "github.com/jasoet/pkg/v3/otel" ) // Mock workflow service client @@ -138,9 +142,49 @@ func (m *mockArgoClient) NewInfoServiceClient() (info.InfoServiceClient, error) return nil, errors.New("not implemented") } +func TestSubmitWorkflow_EmitsSpanFromContextConfig(t *testing.T) { + exporter := tracetest.NewInMemoryExporter() + tp := sdktrace.NewTracerProvider(sdktrace.WithSyncer(exporter)) + t.Cleanup(func() { + assert.NoError(t, tp.Shutdown(context.Background())) + }) + + cfg := otel.NewConfig("test-service", otel.WithTracerProvider(tp)) + ctx := otel.ContextWithConfig(context.Background(), cfg) + + testWf := &v1alpha1.Workflow{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-", + Namespace: "argo", + }, + Spec: v1alpha1.WorkflowSpec{ + Entrypoint: "main", + }, + } + + mockWfClient := &mockWorkflowServiceClient{ + createWorkflowFunc: func(ctx context.Context, req *workflow.WorkflowCreateRequest) (*v1alpha1.Workflow, error) { + created := testWf.DeepCopy() + created.Name = "test-span" + created.UID = "uid-span" + return created, nil + }, + } + client := &mockArgoClient{workflowServiceClient: mockWfClient} + + created, err := SubmitWorkflow(ctx, client, testWf) + require.NoError(t, err) + require.NotNil(t, created) + + spans := exporter.GetSpans() + require.Len(t, spans, 1, "expected exactly one ended span") + assert.Equal(t, "argo.SubmitWorkflow", spans[0].Name) + assert.Equal(t, "github.com/jasoet/pkg/v3/argo", spans[0].InstrumentationScope.Name) +} + func TestSubmitWorkflow(t *testing.T) { - ctx := context.Background() cfg := otel.NewConfig("test") + ctx := otel.ContextWithConfig(context.Background(), cfg) testWf := &v1alpha1.Workflow{ ObjectMeta: metav1.ObjectMeta{ @@ -164,7 +208,7 @@ func TestSubmitWorkflow(t *testing.T) { client := &mockArgoClient{workflowServiceClient: mockWfClient} - created, err := SubmitWorkflow(ctx, client, testWf, cfg) + created, err := SubmitWorkflow(ctx, client, testWf) require.NoError(t, err) require.NotNil(t, created) assert.Equal(t, "test-abc123", created.Name) @@ -180,7 +224,7 @@ func TestSubmitWorkflow(t *testing.T) { client := &mockArgoClient{workflowServiceClient: mockWfClient} - created, err := SubmitWorkflow(ctx, client, testWf, cfg) + created, err := SubmitWorkflow(ctx, client, testWf) require.Error(t, err) assert.Nil(t, created) assert.Contains(t, err.Error(), "failed to submit workflow") @@ -197,7 +241,7 @@ func TestSubmitWorkflow(t *testing.T) { client := &mockArgoClient{workflowServiceClient: mockWfClient} - created, err := SubmitWorkflow(ctx, client, testWf, nil) + created, err := SubmitWorkflow(context.Background(), client, testWf) require.NoError(t, err) require.NotNil(t, created) assert.Equal(t, "test-xyz789", created.Name) @@ -205,8 +249,8 @@ func TestSubmitWorkflow(t *testing.T) { } func TestSubmitAndWait(t *testing.T) { - ctx := context.Background() cfg := otel.NewConfig("test") + ctx := otel.ContextWithConfig(context.Background(), cfg) testWf := &v1alpha1.Workflow{ ObjectMeta: metav1.ObjectMeta{ @@ -242,7 +286,7 @@ func TestSubmitAndWait(t *testing.T) { client := &mockArgoClient{workflowServiceClient: mockWfClient} - completed, err := SubmitAndWait(ctx, client, testWf, cfg, 30*time.Second) + completed, err := SubmitAndWait(ctx, client, testWf, 30*time.Second, WithPollInterval(5*time.Millisecond)) require.NoError(t, err) require.NotNil(t, completed) assert.Equal(t, v1alpha1.WorkflowSucceeded, completed.Status.Phase) @@ -272,14 +316,15 @@ func TestSubmitAndWait(t *testing.T) { client := &mockArgoClient{workflowServiceClient: mockWfClient} - completed, err := SubmitAndWait(ctx, client, testWf, cfg, 30*time.Second) + completed, err := SubmitAndWait(ctx, client, testWf, 30*time.Second, WithPollInterval(5*time.Millisecond)) require.Error(t, err) require.NotNil(t, completed) assert.Equal(t, v1alpha1.WorkflowFailed, completed.Status.Phase) + assert.ErrorIs(t, err, ErrWorkflowFailed) assert.Contains(t, err.Error(), "workflow failed") }) - t.Run("workflow timeout", func(t *testing.T) { + t.Run("workflow timeout wraps deadline and sentinel", func(t *testing.T) { mockWfClient := &mockWorkflowServiceClient{ createWorkflowFunc: func(ctx context.Context, req *workflow.WorkflowCreateRequest) (*v1alpha1.Workflow, error) { created := testWf.DeepCopy() @@ -296,15 +341,100 @@ func TestSubmitAndWait(t *testing.T) { client := &mockArgoClient{workflowServiceClient: mockWfClient} - _, err := SubmitAndWait(ctx, client, testWf, cfg, 1*time.Second) + _, err := SubmitAndWait(ctx, client, testWf, 100*time.Millisecond, WithPollInterval(10*time.Millisecond)) require.Error(t, err) - assert.Contains(t, err.Error(), "timeout") + assert.ErrorIs(t, err, ErrWaitTimeout, "timeout error should match ErrWaitTimeout sentinel") + assert.ErrorIs(t, err, context.DeadlineExceeded, "timeout error should wrap context.DeadlineExceeded") + }) + + t.Run("parent context cancellation is not mislabeled as timeout", func(t *testing.T) { + mockWfClient := &mockWorkflowServiceClient{ + createWorkflowFunc: func(ctx context.Context, req *workflow.WorkflowCreateRequest) (*v1alpha1.Workflow, error) { + created := testWf.DeepCopy() + created.Name = "test-cancel" + return created, nil + }, + getWorkflowFunc: func(ctx context.Context, req *workflow.WorkflowGetRequest) (*v1alpha1.Workflow, error) { + result := testWf.DeepCopy() + result.Name = "test-cancel" + result.Status.Phase = v1alpha1.WorkflowRunning + return result, nil + }, + } + + client := &mockArgoClient{workflowServiceClient: mockWfClient} + + cancelCtx, cancel := context.WithCancel(ctx) + go func() { + time.Sleep(30 * time.Millisecond) + cancel() + }() + + _, err := SubmitAndWait(cancelCtx, client, testWf, 10*time.Second, WithPollInterval(5*time.Millisecond)) + require.Error(t, err) + assert.ErrorIs(t, err, context.Canceled) + assert.NotErrorIs(t, err, ErrWaitTimeout, "a cancellation must not be reported as a timeout") + assert.Contains(t, err.Error(), "canceled") + }) + + t.Run("permanent poll error aborts early", func(t *testing.T) { + callCount := 0 + mockWfClient := &mockWorkflowServiceClient{ + createWorkflowFunc: func(ctx context.Context, req *workflow.WorkflowCreateRequest) (*v1alpha1.Workflow, error) { + created := testWf.DeepCopy() + created.Name = "test-notfound" + return created, nil + }, + getWorkflowFunc: func(ctx context.Context, req *workflow.WorkflowGetRequest) (*v1alpha1.Workflow, error) { + callCount++ + return nil, status.Error(codes.NotFound, "workflows.argoproj.io \"test-notfound\" not found") + }, + } + + client := &mockArgoClient{workflowServiceClient: mockWfClient} + + start := time.Now() + _, err := SubmitAndWait(ctx, client, testWf, 10*time.Second, WithPollInterval(5*time.Millisecond)) + require.Error(t, err) + assert.NotErrorIs(t, err, ErrWaitTimeout, "a permanent NotFound must not spin until timeout") + assert.Contains(t, err.Error(), "failed to get workflow status") + assert.Less(t, time.Since(start), 2*time.Second, "should abort promptly on a permanent error") + assert.Equal(t, 1, callCount, "should not retry a permanent error") + }) + + t.Run("transient poll error is retried", func(t *testing.T) { + callCount := 0 + mockWfClient := &mockWorkflowServiceClient{ + createWorkflowFunc: func(ctx context.Context, req *workflow.WorkflowCreateRequest) (*v1alpha1.Workflow, error) { + created := testWf.DeepCopy() + created.Name = "test-transient" + return created, nil + }, + getWorkflowFunc: func(ctx context.Context, req *workflow.WorkflowGetRequest) (*v1alpha1.Workflow, error) { + callCount++ + if callCount < 3 { + return nil, status.Error(codes.Unavailable, "server temporarily unavailable") + } + result := testWf.DeepCopy() + result.Name = "test-transient" + result.Status.Phase = v1alpha1.WorkflowSucceeded + return result, nil + }, + } + + client := &mockArgoClient{workflowServiceClient: mockWfClient} + + completed, err := SubmitAndWait(ctx, client, testWf, 10*time.Second, WithPollInterval(5*time.Millisecond)) + require.NoError(t, err) + require.NotNil(t, completed) + assert.Equal(t, v1alpha1.WorkflowSucceeded, completed.Status.Phase) + assert.GreaterOrEqual(t, callCount, 3, "transient errors should be retried until success") }) } func TestGetWorkflowStatus(t *testing.T) { - ctx := context.Background() cfg := otel.NewConfig("test") + ctx := otel.ContextWithConfig(context.Background(), cfg) t.Run("successful get", func(t *testing.T) { mockWfClient := &mockWorkflowServiceClient{ @@ -324,7 +454,7 @@ func TestGetWorkflowStatus(t *testing.T) { client := &mockArgoClient{workflowServiceClient: mockWfClient} - status, err := GetWorkflowStatus(ctx, client, "argo", "test-workflow", cfg) + status, err := GetWorkflowStatus(ctx, client, "argo", "test-workflow") require.NoError(t, err) require.NotNil(t, status) assert.Equal(t, v1alpha1.WorkflowSucceeded, status.Phase) @@ -340,7 +470,7 @@ func TestGetWorkflowStatus(t *testing.T) { client := &mockArgoClient{workflowServiceClient: mockWfClient} - status, err := GetWorkflowStatus(ctx, client, "argo", "nonexistent", cfg) + status, err := GetWorkflowStatus(ctx, client, "argo", "nonexistent") require.Error(t, err) assert.Nil(t, status) assert.Contains(t, err.Error(), "failed to get workflow") @@ -359,7 +489,7 @@ func TestGetWorkflowStatus(t *testing.T) { client := &mockArgoClient{workflowServiceClient: mockWfClient} - status, err := GetWorkflowStatus(ctx, client, "argo", "test", nil) + status, err := GetWorkflowStatus(context.Background(), client, "argo", "test") require.NoError(t, err) require.NotNil(t, status) assert.Equal(t, v1alpha1.WorkflowRunning, status.Phase) @@ -367,8 +497,8 @@ func TestGetWorkflowStatus(t *testing.T) { } func TestListWorkflows(t *testing.T) { - ctx := context.Background() cfg := otel.NewConfig("test") + ctx := otel.ContextWithConfig(context.Background(), cfg) t.Run("list all workflows", func(t *testing.T) { mockWfClient := &mockWorkflowServiceClient{ @@ -384,13 +514,50 @@ func TestListWorkflows(t *testing.T) { client := &mockArgoClient{workflowServiceClient: mockWfClient} - workflows, err := ListWorkflows(ctx, client, "argo", "", cfg) + workflows, err := ListWorkflows(ctx, client, "argo", "") require.NoError(t, err) require.Len(t, workflows, 2) assert.Equal(t, "wf-1", workflows[0].Name) assert.Equal(t, "wf-2", workflows[1].Name) }) + t.Run("follows pagination continue tokens", func(t *testing.T) { + call := 0 + mockWfClient := &mockWorkflowServiceClient{ + listWorkflowsFunc: func(ctx context.Context, req *workflow.WorkflowListRequest) (*v1alpha1.WorkflowList, error) { + call++ + switch call { + case 1: + assert.Empty(t, req.ListOptions.Continue, "first page must not send a continue token") + return &v1alpha1.WorkflowList{ + ListMeta: metav1.ListMeta{Continue: "token-page-2"}, + Items: []v1alpha1.Workflow{ + {ObjectMeta: metav1.ObjectMeta{Name: "wf-1"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "wf-2"}}, + }, + }, nil + case 2: + assert.Equal(t, "token-page-2", req.ListOptions.Continue, "second page must send the continue token") + return &v1alpha1.WorkflowList{ + Items: []v1alpha1.Workflow{ + {ObjectMeta: metav1.ObjectMeta{Name: "wf-3"}}, + }, + }, nil + default: + return nil, errors.New("unexpected extra list call") + } + }, + } + + client := &mockArgoClient{workflowServiceClient: mockWfClient} + + workflows, err := ListWorkflows(ctx, client, "argo", "") + require.NoError(t, err) + require.Len(t, workflows, 3, "should aggregate items across all pages") + assert.Equal(t, "wf-3", workflows[2].Name) + assert.Equal(t, 2, call, "should have followed exactly one continue token") + }) + t.Run("list with label selector", func(t *testing.T) { mockWfClient := &mockWorkflowServiceClient{ listWorkflowsFunc: func(ctx context.Context, req *workflow.WorkflowListRequest) (*v1alpha1.WorkflowList, error) { @@ -405,7 +572,7 @@ func TestListWorkflows(t *testing.T) { client := &mockArgoClient{workflowServiceClient: mockWfClient} - workflows, err := ListWorkflows(ctx, client, "argo", "app=myapp", cfg) + workflows, err := ListWorkflows(ctx, client, "argo", "app=myapp") require.NoError(t, err) require.Len(t, workflows, 1) }) @@ -419,7 +586,7 @@ func TestListWorkflows(t *testing.T) { client := &mockArgoClient{workflowServiceClient: mockWfClient} - workflows, err := ListWorkflows(ctx, client, "argo", "", cfg) + workflows, err := ListWorkflows(ctx, client, "argo", "") require.Error(t, err) assert.Nil(t, workflows) assert.Contains(t, err.Error(), "failed to list workflows") @@ -427,8 +594,8 @@ func TestListWorkflows(t *testing.T) { } func TestDeleteWorkflow(t *testing.T) { - ctx := context.Background() cfg := otel.NewConfig("test") + ctx := otel.ContextWithConfig(context.Background(), cfg) t.Run("successful deletion", func(t *testing.T) { mockWfClient := &mockWorkflowServiceClient{ @@ -441,7 +608,7 @@ func TestDeleteWorkflow(t *testing.T) { client := &mockArgoClient{workflowServiceClient: mockWfClient} - err := DeleteWorkflow(ctx, client, "argo", "test-workflow", cfg) + err := DeleteWorkflow(ctx, client, "argo", "test-workflow") require.NoError(t, err) }) @@ -454,7 +621,7 @@ func TestDeleteWorkflow(t *testing.T) { client := &mockArgoClient{workflowServiceClient: mockWfClient} - err := DeleteWorkflow(ctx, client, "argo", "test-workflow", cfg) + err := DeleteWorkflow(ctx, client, "argo", "test-workflow") require.Error(t, err) assert.Contains(t, err.Error(), "failed to delete workflow") }) @@ -468,7 +635,7 @@ func TestDeleteWorkflow(t *testing.T) { client := &mockArgoClient{workflowServiceClient: mockWfClient} - err := DeleteWorkflow(ctx, client, "argo", "test-workflow", nil) + err := DeleteWorkflow(context.Background(), client, "argo", "test-workflow") require.NoError(t, err) }) } diff --git a/argo/option.go b/argo/option.go index 61e3432..64f1040 100644 --- a/argo/option.go +++ b/argo/option.go @@ -1,11 +1,11 @@ package argo import ( - "github.com/jasoet/pkg/v2/otel" + "github.com/jasoet/pkg/v3/otel" ) // Option is a functional option for configuring Argo client. -type Option func(*Config) error +type Option func(*Config) // WithKubeConfig sets the path to the kubeconfig file. // If not set, the default location (~/.kube/config) will be used. @@ -16,9 +16,8 @@ type Option func(*Config) error // argo.WithKubeConfig("/custom/path/to/kubeconfig"), // ) func WithKubeConfig(path string) Option { - return func(c *Config) error { + return func(c *Config) { c.KubeConfigPath = path - return nil } } @@ -31,9 +30,8 @@ func WithKubeConfig(path string) Option { // argo.WithContext("production"), // ) func WithContext(context string) Option { - return func(c *Config) error { + return func(c *Config) { c.Context = context - return nil } } @@ -47,9 +45,8 @@ func WithContext(context string) Option { // argo.WithInCluster(true), // ) func WithInCluster(inCluster bool) Option { - return func(c *Config) error { + return func(c *Config) { c.InCluster = inCluster - return nil } } @@ -62,10 +59,9 @@ func WithInCluster(inCluster bool) Option { // argo.WithArgoServer("https://argo-server:2746", "Bearer token"), // ) func WithArgoServer(url, authToken string) Option { - return func(c *Config) error { + return func(c *Config) { c.ArgoServerOpts.URL = url c.ArgoServerOpts.AuthToken = authToken - return nil } } @@ -80,9 +76,8 @@ func WithArgoServer(url, authToken string) Option { // argo.WithArgoServerInsecure(true), // ) func WithArgoServerInsecure(insecure bool) Option { - return func(c *Config) error { + return func(c *Config) { c.ArgoServerOpts.InsecureSkipVerify = insecure - return nil } } @@ -96,9 +91,8 @@ func WithArgoServerInsecure(insecure bool) Option { // argo.WithArgoServerHTTP1(true), // ) func WithArgoServerHTTP1(http1 bool) Option { - return func(c *Config) error { + return func(c *Config) { c.ArgoServerOpts.HTTP1 = http1 - return nil } } @@ -107,17 +101,17 @@ func WithArgoServerHTTP1(http1 bool) Option { // // Example: // -// otelConfig := otel.NewConfig("my-service"). -// WithTracerProvider(tracerProvider). -// WithMeterProvider(meterProvider) +// otelConfig := otel.NewConfig("my-service", +// otel.WithTracerProvider(tracerProvider), +// otel.WithMeterProvider(meterProvider), +// ) // // ctx, client, err := argo.NewClientWithOptions(ctx, // argo.WithOTelConfig(otelConfig), // ) func WithOTelConfig(otelConfig *otel.Config) Option { - return func(c *Config) error { + return func(c *Config) { c.OTelConfig = otelConfig - return nil } } @@ -137,9 +131,8 @@ func WithOTelConfig(otelConfig *otel.Config) Option { // argo.WithArgoServerOpts(serverOpts), // ) func WithArgoServerOpts(opts ServerOpts) Option { - return func(c *Config) error { + return func(c *Config) { c.ArgoServerOpts = opts - return nil } } @@ -161,8 +154,7 @@ func WithArgoServerOpts(opts ServerOpts) Option { // argo.WithConfig(config), // ) func WithConfig(config *Config) Option { - return func(c *Config) error { + return func(c *Config) { *c = *config - return nil } } diff --git a/argo/option_test.go b/argo/option_test.go index 730b125..e9e4186 100644 --- a/argo/option_test.go +++ b/argo/option_test.go @@ -4,18 +4,16 @@ import ( "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "github.com/jasoet/pkg/v2/otel" + "github.com/jasoet/pkg/v3/otel" ) func TestWithKubeConfig(t *testing.T) { config := &Config{} path := "/custom/path/to/kubeconfig" - err := WithKubeConfig(path)(config) + WithKubeConfig(path)(config) - require.NoError(t, err) assert.Equal(t, path, config.KubeConfigPath) } @@ -23,9 +21,8 @@ func TestWithContext(t *testing.T) { config := &Config{} contextName := "production" - err := WithContext(contextName)(config) + WithContext(contextName)(config) - require.NoError(t, err) assert.Equal(t, contextName, config.Context) } @@ -42,9 +39,8 @@ func TestWithInCluster(t *testing.T) { t.Run(tt.name, func(t *testing.T) { config := &Config{} - err := WithInCluster(tt.inCluster)(config) + WithInCluster(tt.inCluster)(config) - require.NoError(t, err) assert.Equal(t, tt.inCluster, config.InCluster) }) } @@ -55,9 +51,8 @@ func TestWithArgoServer(t *testing.T) { url := "https://argo-server:2746" token := "Bearer test-token" - err := WithArgoServer(url, token)(config) + WithArgoServer(url, token)(config) - require.NoError(t, err) assert.Equal(t, url, config.ArgoServerOpts.URL) assert.Equal(t, token, config.ArgoServerOpts.AuthToken) } @@ -75,9 +70,8 @@ func TestWithArgoServerInsecure(t *testing.T) { t.Run(tt.name, func(t *testing.T) { config := &Config{} - err := WithArgoServerInsecure(tt.insecure)(config) + WithArgoServerInsecure(tt.insecure)(config) - require.NoError(t, err) assert.Equal(t, tt.insecure, config.ArgoServerOpts.InsecureSkipVerify) }) } @@ -96,9 +90,8 @@ func TestWithArgoServerHTTP1(t *testing.T) { t.Run(tt.name, func(t *testing.T) { config := &Config{} - err := WithArgoServerHTTP1(tt.http1)(config) + WithArgoServerHTTP1(tt.http1)(config) - require.NoError(t, err) assert.Equal(t, tt.http1, config.ArgoServerOpts.HTTP1) }) } @@ -108,9 +101,8 @@ func TestWithOTelConfig(t *testing.T) { config := &Config{} otelConfig := otel.NewConfig("test-service") - err := WithOTelConfig(otelConfig)(config) + WithOTelConfig(otelConfig)(config) - require.NoError(t, err) assert.NotNil(t, config.OTelConfig) assert.Equal(t, otelConfig, config.OTelConfig) } @@ -124,9 +116,8 @@ func TestWithArgoServerOpts(t *testing.T) { HTTP1: true, } - err := WithArgoServerOpts(serverOpts)(config) + WithArgoServerOpts(serverOpts)(config) - require.NoError(t, err) assert.Equal(t, serverOpts, config.ArgoServerOpts) assert.Equal(t, serverOpts.URL, config.ArgoServerOpts.URL) assert.Equal(t, serverOpts.AuthToken, config.ArgoServerOpts.AuthToken) @@ -146,9 +137,8 @@ func TestWithConfig(t *testing.T) { }, } - err := WithConfig(newConfig)(config) + WithConfig(newConfig)(config) - require.NoError(t, err) assert.Equal(t, newConfig.KubeConfigPath, config.KubeConfigPath) assert.Equal(t, newConfig.Context, config.Context) assert.Equal(t, newConfig.InCluster, config.InCluster) @@ -158,14 +148,9 @@ func TestWithConfig(t *testing.T) { func TestMultipleOptions(t *testing.T) { config := &Config{} - err := WithKubeConfig("/path/to/kubeconfig")(config) - require.NoError(t, err) - - err = WithContext("production")(config) - require.NoError(t, err) - - err = WithInCluster(false)(config) - require.NoError(t, err) + WithKubeConfig("/path/to/kubeconfig")(config) + WithContext("production")(config) + WithInCluster(false)(config) assert.Equal(t, "/path/to/kubeconfig", config.KubeConfigPath) assert.Equal(t, "production", config.Context) @@ -184,8 +169,7 @@ func TestChainingOptions(t *testing.T) { } for _, opt := range opts { - err := opt(config) - require.NoError(t, err) + opt(config) } assert.Equal(t, "/custom/kubeconfig", config.KubeConfigPath) diff --git a/argo/patterns/cicd.go b/argo/patterns/cicd.go index d90d530..701c715 100644 --- a/argo/patterns/cicd.go +++ b/argo/patterns/cicd.go @@ -1,10 +1,12 @@ package patterns import ( + "fmt" + "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1" - "github.com/jasoet/pkg/v2/argo/builder" - "github.com/jasoet/pkg/v2/argo/builder/template" + "github.com/jasoet/pkg/v3/argo/builder" + "github.com/jasoet/pkg/v3/argo/builder/template" ) // BuildTestDeploy creates a simple CI/CD workflow pattern with build, test, and deploy stages. @@ -98,8 +100,23 @@ echo "Duration: {{workflow.duration}}" Build() } -// ConditionalDeploy creates a workflow that deploys only if tests pass. -// This demonstrates conditional execution using the 'when' clause. +// ConditionalDeploy creates a workflow that deploys only if tests pass and rolls back +// if the deployment fails. It demonstrates conditional execution using the 'when' clause +// combined with 'continueOn' so the rollback step is actually reachable. +// +// Execution semantics: +// - test runs first. If it fails, the workflow aborts (nothing is deployed, so there is +// nothing to roll back) and the workflow is marked Failed. +// - deploy runs only when test Succeeded ({{steps.test.status}} == Succeeded). It sets +// continueOn.failed so that a deploy failure does NOT abort the workflow before the +// rollback step can evaluate its condition. +// - rollback runs only when deploy Failed ({{steps.deploy.status}} == Failed). Because +// deploy tolerates failure via continueOn, the rollback is recovery logic and the +// workflow completes rather than aborting mid-deploy. +// +// Note: gating on step status ({{steps.X.status}}) rather than {{steps.X.outputs.exitCode}} +// avoids depending on the step actually emitting an exitCode output, which Argo only +// populates for templates that declare it. // // Example: // @@ -112,15 +129,17 @@ func ConditionalDeploy(name, namespace, image string, opts ...builder.Option) (* test := template.NewContainer("test", image, template.WithCommand("go", "test", "./...")) - // Deploy only if tests pass + // Deploy only if tests pass. continueOn.failed keeps the workflow running when the + // deploy fails so the rollback step below can evaluate its condition. deploy := template.NewContainer("deploy", image, template.WithCommand("sh", "-c", "echo 'Deploying to production...'")). - When("{{steps.test.outputs.exitCode}} == 0") + When("{{steps.test.status}} == Succeeded"). + ContinueOn(&v1alpha1.ContinueOn{Failed: true}) - // Rollback if deploy fails + // Rollback only if the deploy step failed. rollback := template.NewContainer("rollback", image, template.WithCommand("sh", "-c", "echo 'Rolling back deployment...'")). - When("{{steps.deploy.outputs.exitCode}} != 0") + When("{{steps.deploy.status}} == Failed") return builder.NewWorkflowBuilder(name, namespace, opts...). Add(test). @@ -139,6 +158,10 @@ func ConditionalDeploy(name, namespace, image string, opts ...builder.Option) (* // []string{"staging", "production"}, // ) func MultiEnvironmentDeploy(name, namespace, deployImage string, environments []string, opts ...builder.Option) (*v1alpha1.Workflow, error) { + if len(environments) == 0 { + return nil, fmt.Errorf("at least one environment is required for multi-environment deploy") + } + wb := builder.NewWorkflowBuilder(name, namespace, opts...) // Add deployment step for each environment diff --git a/argo/patterns/cicd_test.go b/argo/patterns/cicd_test.go index 19eef7c..774b862 100644 --- a/argo/patterns/cicd_test.go +++ b/argo/patterns/cicd_test.go @@ -7,7 +7,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/jasoet/pkg/v2/argo/builder" + "github.com/jasoet/pkg/v3/argo/builder" ) func TestBuildTestDeploy(t *testing.T) { @@ -128,23 +128,41 @@ func TestConditionalDeploy(t *testing.T) { require.NotNil(t, mainTemplate) require.NotEmpty(t, mainTemplate.Steps) - // Check for conditional deploy and rollback steps - hasConditionalDeploy := false - hasRollback := false - - for _, parallelSteps := range mainTemplate.Steps { - for _, step := range parallelSteps.Steps { - if step.Name == "deploy" && step.When != "" { - hasConditionalDeploy = true - } - if step.Name == "rollback" && step.When != "" { - hasRollback = true + // Locate the deploy and rollback steps. + var deployStep, rollbackStep *v1alpha1.WorkflowStep + for i := range mainTemplate.Steps { + for j := range mainTemplate.Steps[i].Steps { + step := &mainTemplate.Steps[i].Steps[j] + switch step.Name { + case "deploy": + deployStep = step + case "rollback": + rollbackStep = step } } } - assert.True(t, hasConditionalDeploy, "should have conditional deploy step") - assert.True(t, hasRollback, "should have conditional rollback step") + require.NotNil(t, deployStep, "should have conditional deploy step") + require.NotNil(t, rollbackStep, "should have conditional rollback step") + + // Deploy must gate on test success and tolerate its own failure so rollback is reachable. + assert.Equal(t, "{{steps.test.status}} == Succeeded", deployStep.When) + require.NotNil(t, deployStep.ContinueOn, "deploy must set continueOn so a deploy failure does not abort before rollback") + assert.True(t, deployStep.ContinueOn.Failed, "deploy continueOn.failed must be true") + + // Rollback must gate on deploy failure (via status, not exitCode). + assert.Equal(t, "{{steps.deploy.status}} == Failed", rollbackStep.When) +} + +func TestMultiEnvironmentDeployRequiresEnvironments(t *testing.T) { + _, err := MultiEnvironmentDeploy( + "empty", "argo", + "deployer:v1", + []string{}, + ) + + require.Error(t, err) + assert.Contains(t, err.Error(), "at least one environment is required") } func TestMultiEnvironmentDeploy(t *testing.T) { diff --git a/argo/patterns/parallel.go b/argo/patterns/parallel.go index 14c97c8..816fb05 100644 --- a/argo/patterns/parallel.go +++ b/argo/patterns/parallel.go @@ -2,16 +2,21 @@ package patterns import ( "fmt" + "sort" "strings" "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1" - "github.com/jasoet/pkg/v2/argo/builder" - "github.com/jasoet/pkg/v2/argo/builder/template" + "github.com/jasoet/pkg/v3/argo/builder" + "github.com/jasoet/pkg/v3/argo/builder/template" ) -// shellQuote wraps a string in single quotes and escapes any embedded single quotes, -// preventing shell injection when user-provided values are interpolated into shell commands. +// shellQuote wraps a single data value (a filename, path, or similar argument) in +// single quotes, escaping any embedded single quotes. Use it ONLY for data arguments +// that are interpolated into a shell command — never for the command itself, because a +// user-supplied command such as "wc -w" or "awk '{print}'" is a shell fragment that must +// remain unquoted so the shell parses it into a program plus arguments. Quoting the whole +// fragment would make the shell look for a single executable literally named "wc -w". func shellQuote(s string) string { return "'" + strings.ReplaceAll(s, "'", "'\\''") + "'" } @@ -112,9 +117,11 @@ func ParallelDataProcessing(name, namespace, image string, dataItems []string, p parallelSteps := make([]v1alpha1.WorkflowStep, 0, len(dataItems)) for i, dataItem := range dataItems { taskName := fmt.Sprintf("process-%d", i) + // processingCommand is a shell fragment (e.g. "process.sh" or "python run.py") + // and must stay unquoted; only the data item (a filename) is quoted. task := template.NewContainer(taskName, image, template.WithCommand("sh", "-c"), - template.WithArgs(fmt.Sprintf("%s %s", shellQuote(processingCommand), shellQuote(dataItem))), + template.WithArgs(fmt.Sprintf("%s %s", processingCommand, shellQuote(dataItem))), template.WithEnv("DATA_ITEM", dataItem), template.WithEnv("ITEM_INDEX", fmt.Sprintf("%d", i))) @@ -172,9 +179,11 @@ func MapReduce(name, namespace, image string, inputs []string, mapCmd, reduceCmd mapSteps := make([]v1alpha1.WorkflowStep, 0, len(inputs)) for i, input := range inputs { mapTaskName := fmt.Sprintf("map-%d", i) + // mapCmd is a shell fragment (e.g. "wc -w") and must stay unquoted so the shell + // parses it into a program and arguments; only the input filename is quoted. mapTask := template.NewContainer(mapTaskName, image, template.WithCommand("sh", "-c"), - template.WithArgs(fmt.Sprintf("echo 'Mapping %s' && %s %s", shellQuote(input), shellQuote(mapCmd), shellQuote(input))), + template.WithArgs(fmt.Sprintf("echo 'Mapping %s' && %s %s", shellQuote(input), mapCmd, shellQuote(input))), template.WithEnv("INPUT", input)) steps, err := mapTask.Steps() @@ -194,10 +203,11 @@ func MapReduce(name, namespace, image string, inputs []string, mapCmd, reduceCmd } } - // Reduce phase: Aggregate results + // Reduce phase: Aggregate results. reduceCmd is a shell fragment + // (e.g. "awk '{sum+=$1} END {print sum}'") and must stay unquoted. reduce := template.NewContainer("reduce", image, template.WithCommand("sh", "-c"), - template.WithArgs(fmt.Sprintf("echo 'Reducing results...' && %s", shellQuote(reduceCmd)))) + template.WithArgs(fmt.Sprintf("echo 'Reducing results...' && %s", reduceCmd))) reduceSteps, err := reduce.Steps() if err != nil { @@ -247,12 +257,24 @@ func ParallelTestSuite(name, namespace, image string, testSuites map[string]stri wb := builder.NewWorkflowBuilder(name, namespace, opts...) + // Iterate suites in a stable (sorted) order so the generated workflow is + // deterministic across runs — Go map iteration order is randomized, which would + // otherwise break GitOps diffing and reproducible builds. + suiteNames := make([]string, 0, len(testSuites)) + for suiteName := range testSuites { + suiteNames = append(suiteNames, suiteName) + } + sort.Strings(suiteNames) + // Create parallel test steps parallelSteps := make([]v1alpha1.WorkflowStep, 0, len(testSuites)) - for suiteName, testCmd := range testSuites { + for _, suiteName := range suiteNames { + testCmd := testSuites[suiteName] + // testCmd is a shell fragment (e.g. "go test ./...") and must stay unquoted; + // only the suite name (interpolated into an echo) is quoted. testTask := template.NewContainer("test-"+suiteName, image, template.WithCommand("sh", "-c"), - template.WithArgs(fmt.Sprintf("echo 'Running %s tests...' && %s", shellQuote(suiteName), shellQuote(testCmd))), + template.WithArgs(fmt.Sprintf("echo 'Running %s tests...' && %s", shellQuote(suiteName), testCmd)), template.WithWorkingDir("/workspace")) steps, err := testTask.Steps() diff --git a/argo/patterns/parallel_test.go b/argo/patterns/parallel_test.go index f285ed8..ce64a48 100644 --- a/argo/patterns/parallel_test.go +++ b/argo/patterns/parallel_test.go @@ -7,9 +7,26 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/jasoet/pkg/v2/argo/builder" + "github.com/jasoet/pkg/v3/argo/builder" ) +// containerArgs returns the single Args entry of the container template with the given +// name. Patterns generate one `sh -c "