cli: add debug_zip_command_flags.txt file in debug zip

[aa-joshi]

Previously, debug zip doesn't have any way of determining the flags set during command execution. It is a pain point for support team to figure out flag values passesd during debug zip generation. To address this, we have added debug_zip_command_flags.txt file in debug zip. It captures flag values set during command execution.

Epic: CRDB-32134
Release note: None

[cockroach-teamcity]

This change is 

[arjunmahishi]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aa-joshi and @angles-n-daemons)

---

pkg/cli/zip.go line 371 at r1 (raw file):

			s = zr.start("capture debug zip flags")
			if err := z.createJSON(s, zc.prefix+"/debug_zip_command_flags.json", zipCtx); err != nil {

Have you considered reading the provided args from *cobra.Command? it will have all the flags that the user passed. That would significantly reduce the size of the diff.

[aa-joshi]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @angles-n-daemons and @arjunmahishi)

---

pkg/cli/zip.go line 371 at r1 (raw file):

Previously, arjunmahishi (Arjun Mahishi) wrote…

Have you considered reading the provided args from *cobra.Command? it will have all the flags that the user passed. That would significantly reduce the size of the diff.

Thanks for the suggestion! I did explore that cobra command. The challenge with that is we have to iterate over flags to fetch the value & dump into file.
Coming back to diff, The only change is to export field for generating json file. Most of the files are from test case updates.
I can split test & code changes in different commits.
Let me know your thoughts.

[arjunmahishi]

pkg/cli/zip.go line 371 at r1 (raw file):

The challenge with that is we have to iterate over flags to fetch the value & dump into file.

Should be pretty straightforward. I think there is a helper function specifically built for this kind of usecase.

cmd.Flags().VisitAll(func(f *pflag.Flag) {
  if f.Changed {
    // this condition is true only if the user has set the flag
    // add to a string builder
  }
})

Other than the diff size, IMO this is a safer change. Sometimes toggling a struct field between exported/un-exported can have unexpected side effects.

Either way, I think it will be good to add a datadriven test that asserts the file generated.

[aa-joshi]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @angles-n-daemons and @arjunmahishi)

---

pkg/cli/zip.go line 371 at r1 (raw file):

Previously, arjunmahishi (Arjun Mahishi) wrote…

The challenge with that is we have to iterate over flags to fetch the value & dump into file.

Should be pretty straightforward. I think there is a helper function specifically built for this kind of usecase.

cmd.Flags().VisitAll(func(f *pflag.Flag) {
  if f.Changed {
    // this condition is true only if the user has set the flag
    // add to a string builder
  }
})

Other than the diff size, IMO this is a safer change. Sometime toggling a struct field between exporter/un-exported can have unexpected side effects.

Either way, I think it will be good to add a datadriven test that asserts the file generated.

Suggested changes are updated in latest commit. PTAL. TIA.

[arjunmahishi]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aa-joshi and @angles-n-daemons)

---

pkg/cli/zip.go line 336 at r2 (raw file):

			// the profiles.
			{
				s := zc.clusterPrinter.start("pprof summary script")

Last question: Was this an intentional change? Removing the : (increasing the scope of s)

[aa-joshi]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @angles-n-daemons and @arjunmahishi)

---

pkg/cli/zip.go line 336 at r2 (raw file):

Previously, arjunmahishi (Arjun Mahishi) wrote…

Last question: Was this an intentional change? Removing the : (increasing the scope of s)

Yes, it is intentional. nogo was throwing error due to re initialisation of s.

[aa-joshi]

TFTR!

bors r+

[craig]

Build failed:

• unit_tests

[celiala]

Build failed:

• unit_tests

unit_tests passed on re-try.

bors r+

[craig]

Build succeeded:

• windows_build
• unit_tests
• linux_amd64_fips_build
• acceptance
• linux_amd64_build
• macos_amd64_build
• examples_orms
• local_roachtest_fips
• check_generated_code
• macos_arm64_build
• lint
• docker_image_amd64
• local_roachtest
• linux_arm64_build