Azure / static-web-apps-cli

Azure Static Web Apps CLI ✨
https://aka.ms/swa/cli-local-development
MIT License
585 stars 112 forks source link

feat(telemetry): added new flags and env variable for opting out #656

Closed rupareddy5-21 closed 1 year ago

rupareddy5-21 commented 1 year ago

Added commands to disable/enable telemetry capturing. Command would look like swa telemetry --disable/enable. Also added an environment variable SWA_DISABLE_TELEMETRY where users can set the value to true/false. Users can check the status of telemetry capturing using the command swa telemetry --status. Only one of these flags can be used at a time.

sgollapudi77 commented 1 year ago

On a second thought, if we're just gonna have two flags, one to --enable/disable and one for --status why do we need to have a separate sub command? Can't we just have it as two flags under the main 'swa' command like '--telemetry' (takes bool as input for enabling/disabling the telemetry. Ex: swa --telemetry=false) and another flag like swa --telemetry-status to show the status. What do you think about this @sulabh-msft and @manekinekko ?

Reshmi-Sriram commented 1 year ago

On a second thought, if we're just gonna have two flags, one to --enable/disable and one for --status why do we need to have a separate sub command? Can't we just have it as two flags under the main 'swa' command like '--telemetry' (takes bool as input for enabling/disabling the telemetry. Ex: swa --telemetry=false) and another flag like swa --telemetry-status to show the status. What do you think about this @sulabh-msft and @manekinekko ?

@rupareddy5-21 and I discussed about this. While it is an easier implementation, it has a couple of catches.

Due to all this, we thought having a separate command group will be the cleanest and most user-friendly way of doing this.

sgollapudi77 commented 1 year ago

@rupareddy5-21 and I discussed about this. While it is an easier implementation, it has a couple of catches.

  • if it is limited to just a global flag usage, i.e. swa --telemetry=false, we already have swa itself as an executable command and we'll have to make code changes to ensure if --telemetry is used then we shouldn't trigger swa.
  • If it is a part of a string of other flags, recognizing it as a separate globally actionable flag will be tricky
  • Also, since this is regarding telemetry capturing, this flag will have to take priority over other flags and thus if it is present, we need to add rules to not capture telemetry even for the command when it is called out (e.g. swa start ./dist --api-location=api --telemetry=false needs us to not capture the swa start in the telemetry as well

Due to all this, we thought having a separate command group will be the cleanest and most user-friendly way of doing this.

Replying to your points

  1. 'swa' itself is an executable but invoking swa --telemetry will be nothing different than invoking swa --version which is a global flag.
  2. This can be avoided by checking the other existing flags and renaming appropriately.
  3. We can prevent this from happening when parsing the options for subcommand. Basically, we can prevent subcommand from invoking this.

Upon saying this, I think since this is a big change it'd be better to have a discussion before merging this.

sgollapudi77 commented 1 year ago

Please get the PR reviewed by Vamsi as well.

Rupa, can you resolve the previous comments?