Skip to content

Add describe changed command to check if a package needs rebuild#339

Merged
geropl merged 2 commits intomainfrom
gpl/describe-changed-command
Mar 16, 2026
Merged

Add describe changed command to check if a package needs rebuild#339
geropl merged 2 commits intomainfrom
gpl/describe-changed-command

Conversation

@geropl
Copy link
Copy Markdown
Member

@geropl geropl commented Mar 13, 2026

Description

Adds leeway describe changed <package> — a lightweight command that checks whether a package's current version hash exists in the local or remote cache.

Exit codes:

  • 0: package is cached (unchanged, no rebuild needed)
  • 1: package is not cached (changed, needs rebuild)
  • 2: an error occurred (cache setup failure, version computation error, etc.)

This enables CI branching decisions without running a full build --dry-run across the entire workspace:

if leeway describe changed my-component:my-package; then
  echo "unchanged, skipping build"
else
  echo "changed, rebuilding"
  leeway build my-component:my-package
fi

The command reuses the existing LocalCache.Location() and RemoteCache.ExistingPackages() infrastructure, so it respects the same LEEWAY_CACHE_DIR, LEEWAY_REMOTE_CACHE_BUCKET, and LEEWAY_REMOTE_CACHE_STORAGE configuration as leeway build.

Output is a tab-separated line: <package>\t<version>\t<status> where status is one of cached locally, cached remotely, or changed.

Related Issue(s)

Fixes CORE-

How to test

# Build leeway
go build -o leeway .

# Check a package that hasn't been built — should exit 1
./leeway describe changed //:app
echo $?  # 1

# Build a package
./leeway build //:helloworld

# Check again — should exit 0
./leeway describe changed //:helloworld
echo $?  # 0

Adds a new subcommand that checks whether a package's current version
exists in the local or remote cache. Exits 0 if cached (unchanged),
1 if not (needs rebuild). Intended for CI branching decisions.

Co-authored-by: Ona <no-reply@ona.com>
@geropl geropl marked this pull request as ready for review March 13, 2026 19:18
@geropl geropl requested a review from WVerlaek March 16, 2026 08:37
Copy link
Copy Markdown
Member

@WVerlaek WVerlaek left a comment

Choose a reason for hiding this comment

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

Ona also had some other comments that I think make sense, will post them below

return
}
if pkg == nil {
log.Fatal("changed requires a package, not a component")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How would you differentiate a fatal error like this from "changed"? Both exit with code 1

@WVerlaek
Copy link
Copy Markdown
Member

PR Review: describe changed command

Summary

Single-file addition (cmd/describe-changed.go, 87 lines) that adds leeway describe changed <package> to check if a package's version hash exists in the local or remote cache. Clean, well-scoped feature. A few issues worth addressing:


❌ Exit code semantics are inverted from convention

The command uses exit code 1 to mean "changed / needs rebuild." Exit code 1 is conventionally an error, and tools like set -e, && chains, and CI systems treat non-zero exits as failures. This makes the CI example in the PR description work (if leeway describe changed ...), but it will break in any script using set -e (which is extremely common in CI), because the "changed" case will abort the script.

Consider using exit code 0 for both cases and letting callers branch on the output, or use a dedicated non-error exit code (e.g., exit 2 for "changed") so set -e scripts can use || true patterns more intentionally. Alternatively, document clearly that callers must handle the non-zero exit, and consider using RunE instead of Run to avoid cobra interpreting the exit.

That said, git diff --quiet uses the same pattern (exit 1 = differences found), so there is precedent. If you keep this, it should be explicitly documented that set -e scripts need || true or the if pattern.


⚠️ Silent failure when getTarget returns !exists

_, pkg, _, exists := getTarget(args, false)
if !exists {
    return
}

When getTarget returns !exists, the function returns with no output and exit code 0. This is misleading — a non-existent package silently reports as "cached." Looking at getTarget, it calls log.Fatalf internally when a package doesn't exist, so the !exists branch is likely dead code. But if it's kept for safety, it should os.Exit(1) or log.Fatal, not silently succeed.

Other describe subcommands (e.g., describe-manifest.go) ignore the exists return value entirely and just check pkg == nil. Consider aligning with that pattern.


⚠️ Duplicated local cache setup logic

Lines 52–59 duplicate the local cache initialization from getBuildOpts in build.go (lines 286–296):

localCacheLoc := os.Getenv(leeway.EnvvarCacheDir)
if localCacheLoc == "" {
    localCacheLoc = filepath.Join(os.TempDir(), "leeway", "cache")
}
localCache, err := local.NewFilesystemCache(localCacheLoc)

This is a maintenance risk — if the default cache path logic changes in getBuildOpts, this copy will drift. Consider extracting a shared helper like getLocalCache() *local.FilesystemCache that both build.go and this command can use.


⚠️ Remote cache error is downgraded to a warning but still exits 1

if err != nil {
    log.WithError(err).Warn("cannot check remote cache, assuming changed")
    fmt.Printf("%s\t%s\tchanged\n", pkg.FullName(), version)
    os.Exit(1)
}

This conflates "remote cache is unreachable" with "package has changed." A transient network error will cause CI to rebuild packages unnecessarily. The Warn log helps, but the tab-separated output says changed with no indication that the remote check failed. Consider a distinct status like unknown or error, or at minimum mention the failure in the output line.


Minor nits

  1. No --format / -o flag support: Other describe subcommands support --format (json/yaml/template) via getWriterFromFlags. This command outputs raw tab-separated text only. For consistency and scriptability (especially --format json), consider supporting the standard format flags.

  2. os.Exit in cobra Run: Direct os.Exit calls bypass cobra's shutdown hooks and any deferred functions. The codebase does use log.Fatal extensively (which also calls os.Exit), so this is consistent with existing patterns, but worth noting.

  3. File naming: The file is describe-changed.go, consistent with describe-const.go, describe-manifest.go, etc. ✅


What looks good

  • Reuses existing infrastructure (getTarget, getRemoteCacheFromEnv, local.NewFilesystemCache) rather than reinventing.
  • Well-written command help text with a practical CI example.
  • Correct use of cobra.ExactArgs(1).
  • Tab-separated output is easy to parse programmatically.
  • Checking local cache before remote is the right order (fast path first).

Addresses review feedback: errors now exit with code 2 so CI scripts
can distinguish 'package changed' (exit 1) from 'something went wrong'
(exit 2).

Co-authored-by: Ona <no-reply@ona.com>
@geropl
Copy link
Copy Markdown
Member Author

geropl commented Mar 16, 2026

Addressed in 19c4551 — exit codes are now:

  • 0 — cached (unchanged)
  • 1 — changed (needs rebuild)
  • 2 — error

Errors within the command's own logic (version computation, cache setup, remote cache check) now use log.Error + os.Exit(2) instead of log.Fatal.

Note: getTarget still uses log.Fatal (exit 1) for invalid package names — this is a shared function across all describe subcommands and changing it would be a larger refactor. In practice this is fine since an invalid package name produces a clear error message on stderr, distinguishing it from the "changed" output on stdout.

@geropl geropl requested a review from WVerlaek March 16, 2026 14:11
@geropl geropl merged commit aa69503 into main Mar 16, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants