Skip to content

Remove deprecated lstk config profile command#360

Open
gtsiolis wants to merge 2 commits into
mainfrom
pro-360-remove-deprecated-lstk-config-profile-command
Open

Remove deprecated lstk config profile command#360
gtsiolis wants to merge 2 commits into
mainfrom
pro-360-remove-deprecated-lstk-config-profile-command

Conversation

@gtsiolis

@gtsiolis gtsiolis commented Jul 2, 2026

Copy link
Copy Markdown
Member

What

Removes lstk config profile, a deprecated backward-compat alias for lstk setup aws.

Why

config profile was introduced already-deprecated in #146, but the "Deprecated: use 'lstk setup aws' instead" notice lived only in --help — running it gave no runtime nudge (flagged by DevRel). It is interactive-only (hard-errors in non-interactive mode, so it can't appear in scripts or CI) and a strict subset of setup aws; both delegate to the same handler.

We considered DevRel's suggestion of Cobra's built-in Deprecated field but rejected it: it prints raw to os.Stderr (bypassing our output.Sink layer) and silently strips the command from --help and generated lstk docs. Removing the alias outright is cleaner.

Changes

  • Remove newConfigProfileCmd; simplify newConfigCmd wiring (drop the now-unused env/ui deps).
  • Rename ui.RunConfigProfileui.RunSetupAWS (only setup aws calls it now; mirrors ui.RunSetupAzure).
  • Drop the config profile mentions in internal/awsconfig and CLAUDE.md.
  • Tests: migrate the decline-path test to setup aws (no equivalent existed); drop the confirm-path duplicate (identical to TestSetupAWSCreatesAWSProfileWhenConfirmed); add an exit-code assertion that config profile now exits non-zero with unknown command "profile".

Testing

  • make build, go vet -C test/integration ./..., and unit tests for cmd/, internal/ui/, internal/awsconfig/ — all pass.
  • Integration subset (no Docker): TestSetupAWSDoesNotCreateAWSProfileWhenDeclined, TestSetupAWSCreatesAWSProfileWhenConfirmed, TestInvalidUsageExitsNonZero (incl. removed_config_profile_subcommand), TestBareParentCommandExitsZero — 18/18 pass.

Closes PRO-360

`config profile` was introduced already-deprecated in #146 as a
backward-compat alias for `lstk setup aws`, but emitted no runtime
deprecation warning — the notice lived only in `--help`. It is
interactive-only (errors in non-interactive mode, so it can't appear in
scripts or CI) and a strict subset of `setup aws`; both delegate to the
same handler. Rather than add a louder warning (Cobra's Deprecated field
bypasses output.Sink and hides the command from --help and generated
docs), remove the alias.

- Drop newConfigProfileCmd; simplify newConfigCmd wiring.
- Rename ui.RunConfigProfile -> ui.RunSetupAWS (mirrors ui.RunSetupAzure).
- Migrate the decline-path integration test to `setup aws`; drop the
  confirm-path duplicate; assert `config profile` now exits non-zero.
@gtsiolis gtsiolis requested a review from a team as a code owner July 2, 2026 15:56
@gtsiolis gtsiolis self-assigned this Jul 2, 2026
@gtsiolis gtsiolis added semver: patch docs: skip Pull request does not require documentation changes labels Jul 3, 2026
TestAzStopInterceptionNoOpWhenNotIntercepting flaked on windows-latest with
"TempDir RemoveAll cleanup ... being used by another process". The
interception flow deliberately leaves the global Azure config's telemetry prefs
untouched, so `az` spawns a detached background telemetry-upload process that
outlives the command (CREATE_NEW_CONSOLE, no wait) and keeps a handle inside the
test's temp HOME, breaking t.TempDir() cleanup on Windows.

Set AZURE_CORE_COLLECT_TELEMETRY=false for the test's az invocations so the CLI
never records telemetry and never spawns the uploader. Pre-existing flake on
main, unrelated to the config-profile removal.

@anisaoshafi anisaoshafi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Two small comments, otherwise LGTM!

// config key. Tests that run `az` against a temp HOME set it to "false" so the CLI
// does not spawn its detached background telemetry-upload process, which on Windows
// outlives `az` and keeps a handle inside the temp dir — breaking t.TempDir() cleanup.
AzureCollectTelemetry Key = "AZURE_CORE_COLLECT_TELEMETRY"

@anisaoshafi anisaoshafi Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

what was the reason for the az related code changes in this PR, like this. seems like outside the scope?

updated: actually entire commit 28774fa seems outside the scope of this PR. Isn't this already handled in this newer PR? in that case can we remove this from here to keep things clean? 🙌🏼

Comment thread CLAUDE.md
- `lstk az start-interception` / `lstk az stop-interception` — Opt-in second mode: instead of the isolated dir, these mutate the user's **global** `~/.azure` so plain `az` (any terminal/script) targets LocalStack, then switch back. `start-interception` runs the same register → activate → `instance_discovery=false` → dummy-login flow against the global config (but does not touch global telemetry/survey prefs) and is independent of `lstk setup azure`. `stop-interception` switches the active cloud back to `AzureCloud` (override with `--cloud <name>`, validated against the live `az cloud list`) and re-enables instance discovery — but only if `LocalStack` is still the active cloud, to avoid clobbering an unrelated selection.

This naming avoids AWS-specific "profile" terminology and uses a clear verb for mutation operations.
The deprecated `lstk config profile` command still works but points users to `lstk setup aws`.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do you think it's a good idea to add in CLAUDE.md the decision around not using Cobra's deprecated field, as explained in the PR description already?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs: skip Pull request does not require documentation changes semver: patch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants