[anisaoshafi]

Motivation

Today, App Inspector is turned on with a button in the Web App. This PR adds a CLI command for it, so it can be turned on from the terminal, in CI, or in a setup script.

Solution

• New command: lstk inspect enable | disable. AWS emulator only.
• It turns App Inspector on or off on the running emulator. The change lasts until the emulator restarts.
• Supports --json and --endpoint-url.

Docs

New command lstk inspect (AWS emulator only):

• lstk inspect enable turns App Inspector on.
• lstk inspect disable turns it off.

Both need a running emulator, and the change lasts until the emulator restarts.

Both commands support --json. The output shape and error codes are in docs/structured-output.md.

Review

Human review advised: a new user-facing command.

Closes DEVX-1151

[peter-smith-phd]

Hi @anisaoshafi, this is the first time I've seen this new feature idea, so I have a couple of questions:

1. The lstk inspect command is new. Have we considered other App Inspector-related features that could be accessed via lstk inspect? We don't need to build them now, but I just want to make sure the enable | disable | status syntax you've chosen for this feature also allows us to add new sub-commands in the future, in a meaningful way, without confusing the user. For example, lstk inspect show ... or lstk inspect clear.

2. The --persist option surprised me a bit. At first I thought it was the same --persist flag that lstk start provides, but then I realized that it's completely different. Also, we don't generally use the --persist flag for other commands, but we instead just expect the user to edit the config.toml file directly. Is there a reason for making this a special case? (I hope not, since special cases can be confusing).

[anisaoshafi]

Hi @anisaoshafi, this is the first time I've seen this new feature idea, so I have a couple of questions:

1. The lstk inspect command is new. Have we considered other App Inspector-related features that could be accessed via lstk inspect? We don't need to build them now, but I just want to make sure the enable | disable | status syntax you've chosen for this feature also allows us to add new sub-commands in the future, in a meaningful way, without confusing the user. For example, lstk inspect show ... or lstk inspect clear.
2. The --persist option surprised me a bit. At first I thought it was the same --persist flag that lstk start provides, but then I realized that it's completely different. Also, we don't generally use the --persist flag for other commands, but we instead just expect the user to edit the config.toml file directly. Is there a reason for making this a special case? (I hope not, since special cases can be confusing).

@peter-smith-phd you can check some conversations in here.

Now I see what you mean with --persist having a different meaning with it being a localstack feature in lstk start, while here in inspector is more like saving a setting, I think I went to far with it. My idea was to sort of have a way to save this setting, but I don't think we need it right now. Will consider removing it.

[peter-smith-phd]

Thanks for sharing the previous discussion - I had missed that. I think I'm convinced that lstk inspect can be extended in future, and that lstk inspect enable | disable won't conflict with anything. However, I don't think we've thought about all the inspect subcommands we could add in the future, but we don't need to do that now anyway.

However, lstk inspect status might need some thought, since when we add new features in the future, the status keyword might lead to expectations that we'll show a lot more than just "it's enabled". But, perhaps we could extend status in future, by just adding extra fields.

BTW, does lstk --json inspect status work?

[anisaoshafi]

However, lstk inspect status might need some thought, since when we add new features in the future, the status keyword might lead to expectations that we'll show a lot more than just "it's enabled". But, perhaps we could extend status in future, by just adding extra fields.

Thanks for challenging this, Peter. Agree, it doesn't do that much, we can as well remove the status subcommand. App Inspector can already be enabled via setting it in the environment variable in the config profile, and and lstk start picks it up, but this command makes this more dynamic.

BTW, does lstk --json inspect status work?

Yes, this is the output. Also added one tests to reflect this contract:
{"schemaVersion":1,"command":"inspect status","status":"ok","data":{"changed":false,"enabled":true},"warnings":[],"error":null}

I'll merge this for now, but happy to make further adjustments before the next lstk release 🙏🏼

[peter-smith-phd]

So what exactly is being merged? Is the PR description still accurate?

• I'm not sure I agree with the --persist flag, since that's not a pattern we've adopted in other places.

• The JSON output for status worries me a bit because the changed field will become highly ambiguous if we add more status fields in the future. Right now it's indicating if enabled changes, but in future what does it mean?

[anisaoshafi]

So what exactly is being merged? Is the PR description still accurate?

• I'm not sure I agree with the --persist flag, since that's not a pattern we've adopted in other places.
• The JSON output for status worries me a bit because the changed field will become highly ambiguous if we add more status fields in the future. Right now it's indicating if enabled changes, but in future what does it mean?

I'll remove both --persist and status for now. If and when we need them we'll have more clarity on them.