Skip to content

docs: clarify repo flag requirements and stable ID guidance#791

Open
pbeckham wants to merge 1 commit intomainfrom
5357-clarify-repo-flags-documentation
Open

docs: clarify repo flag requirements and stable ID guidance#791
pbeckham wants to merge 1 commit intomainfrom
5357-clarify-repo-flags-documentation

Conversation

@pbeckham
Copy link
Copy Markdown
Contributor

@pbeckham pbeckham commented Apr 16, 2026

  • Change --repo-id, --repo-url, --repository from [optional] to [conditional] in attestation and trail contexts, explaining that all three must be set together to record repository information
  • Add attestationRepoNameFlag constant to distinguish the attestation context from the PR-lookup context (repositoryFlag)
  • Clarify --repo-id should be a stable VCS provider ID, not the repo name, as names can change on rename
  • Extend commitDescription and beginTrailLongDesc to note that repo flags are auto-set in GitHub Actions, GitLab CI, Bitbucket Pipelines, and Azure DevOps, and must be set explicitly in other CI systems
    https://github.com/kosli-dev/server/issues/5357

- Change --repo-id, --repo-url, --repository from [optional] to
  [conditional] in attestation and trail contexts, explaining that all
  three must be set together to record repository information
- Add attestationRepoNameFlag constant to distinguish the attestation
  context from the PR-lookup context (repositoryFlag)
- Clarify --repo-id should be a stable VCS provider ID, not the repo
  name, as names can change on rename
- Extend commitDescription and beginTrailLongDesc to note that repo
  flags are auto-set in GitHub Actions, GitLab CI, Bitbucket Pipelines,
  and Azure DevOps, and must be set explicitly in other CI systems

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 16, 2026

Claude finished @pbeckham's task in 2m 23s —— View job


PR Review: docs: clarify repo flag requirements and stable ID guidance

  • Run static analysis (build, vet, mod tidy)
  • Read and review changed files
  • Post review feedback

Static Analysis

All checks pass:

  • go build ./... — compiles cleanly
  • go vet ./... — no issues
  • go mod tidy — no dependency drift

Review Summary

This is a well-scoped documentation-only PR that clarifies previously ambiguous flag behaviour. The changes are consistent with the existing codebase patterns and the runtime logic in mergeGitRepoInfo (which already returns nil and warns when ID, Name, or URL is missing).

What's good:

  • Introducing attestationRepoNameFlag to distinguish the attestation/trail --repository context from the PR-lookup repositoryFlag is the right call — these flags share the same CLI name but have different semantics, and their help text should reflect that.
  • The [optional][conditional] relabelling accurately reflects the all-or-nothing constraint already enforced in mergeGitRepoInfo.
  • Cleaning up trailing whitespace in attestArtifact.go and beginTrail.go is a nice incidental fix.
  • Adding CI auto-detection context to the long descriptions is practical — users in non-supported CI systems now know they need to set flags explicitly.

Minor suggestions (see inline comments):

  1. repoURLFlag wording — the original said "Must be a valid URL if provided"; the new text drops "if provided", which could read as "this must always be provided". Consider restoring the qualifier ("Must be a valid URL when provided") for clarity.

  2. No validation for partial sets — The docs now clearly state all three must be set together, and mergeGitRepoInfo silently returns nil + warns when they're not. This is fine for now, but a future slice could add explicit validation that errors (or warns more prominently) when only 1 or 2 of the three are provided. Not blocking.

No concerns with:

  • Code quality and consistency
  • Security implications (no logic changes)
  • Test coverage (existing tests in attestArtifact_test.go, beginTrail_test.go, attestGeneric_test.go, and attestation_test.go exercise the repo flags; since this is docs-only, no new tests are needed)

LGTM with the one minor wording nit on repoURLFlag.

Comment thread cmd/kosli/root.go
Comment thread cmd/kosli/root.go
Copy link
Copy Markdown
Contributor

@mbevc1 mbevc1 left a comment

Choose a reason for hiding this comment

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

Looks good, but tests are failing and needs checking

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