Skip to content

fix: deprecate --visibility flag instead of removing it#923

Merged
ToreMerkely merged 1 commit into
mainfrom
flow-visibility-fix
Jun 1, 2026
Merged

fix: deprecate --visibility flag instead of removing it#923
ToreMerkely merged 1 commit into
mainfrom
flow-visibility-fix

Conversation

@ToreMerkely
Copy link
Copy Markdown
Contributor

Removing --visibility in #918 made passing the flag illegal (unknown
flag), breaking existing scripts. Restore it as a deprecated flag so it
is accepted with a deprecation notice rather than erroring.

Drop the hardcoded visibility "private" added in #922 as a backwards-
compat shim for servers that still required the field; prod no longer
requires it. The field is now sent only when the user explicitly passes
the deprecated flag (json omitempty), preserving compatibility with
older instances that still mandate it.

Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com

Removing --visibility in #918 made passing the flag illegal (unknown
flag), breaking existing scripts. Restore it as a deprecated flag so it
is accepted with a deprecation notice rather than erroring.

Drop the hardcoded visibility "private" added in #922 as a backwards-
compat shim for servers that still required the field; prod no longer
requires it. The field is now sent only when the user explicitly passes
the deprecated flag (json omitempty), preserving compatibility with
older instances that still mandate it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ToreMerkely ToreMerkely enabled auto-merge (squash) June 1, 2026 06:16
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Jun 1, 2026

Claude finished @ToreMerkely's task in 2m 8s —— View job


PR Review: Deprecate --visibility flag instead of removing it

  • Run static analysis (build, vet, mod tidy)
  • Read changed files and review code
  • Check test coverage
  • Post review findings

Static Analysis

All checks pass cleanly:

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

Review Summary

This is a clean, well-scoped fix. Removing --visibility entirely in #918 turned an accepted flag into an unknown-flag error, breaking existing scripts. This PR correctly restores it as a deprecated flag with a deprecation notice.

What's good:

  • The json:"visibility,omitempty" tag is the right approach — the field is only serialized when the user explicitly passes --visibility, avoiding unnecessary payload for modern servers while keeping older instances working
  • The MarkDeprecated + error handling pattern matches the existing precedent in createEnvironment.go:106-109
  • The test case validates both that the flag is accepted and that the deprecation notice appears
  • The flag description in root.go is clearly marked [deprecated], consistent with how other flag descriptions are prefixed

One thing to verify (not blocking):
Removing the hardcoded Visibility = "private" means users on older self-hosted Kosli instances that still require the field will need to pass --visibility private explicitly (with a deprecation warning). The PR description says prod no longer requires it, so this should be fine — but worth confirming that self-hosted/on-prem users have been considered.

Minor nit: posted as an inline comment on the MarkDeprecated block — there's an existing DeprecateFlags helper in cli_utils.go that could be used, but the inline approach matches existing code, so it's a take-it-or-leave-it suggestion.

Overall: Looks good to merge. The approach is clean, the test coverage is sufficient, and it solves the backwards compatibility issue without overcomplicating things.

Comment thread cmd/kosli/createFlow.go
@ToreMerkely ToreMerkely merged commit 0bf74d0 into main Jun 1, 2026
18 checks passed
@ToreMerkely ToreMerkely deleted the flow-visibility-fix branch June 1, 2026 06:24
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