playground: deprecate bootstrap option --monitor and add a new option --without-monitor to disable monitors.

[LittleFall]

What problem does this PR solve?

What --monitor Start prometheus and grafana component means is unclear, tiup playground nightly and tiup playground nightly --monitor has no difference.

What is changed and how it works?

1. deprecate option --monitor, after this pr, if you add this, it still work but will report Flag --monitor has been deprecated, Please use --without-monitor to control whether to disable monitor.
2. add option --without-monitor, in help messagae: --without-monitor Don't start prometheus and grafana component . by the way, help message won't show --mointor anymore.
3. when the two option occurs at the same time, the last occured will work.

Check List

Tests

• Integration test

Related changes

• Need to cherry-pick to the release branch
• Need to update the documentation

Release notes:

deprecate bootstrap option --monitor and add a new option --without-monitor to disable monitors.

[ti-chi-bot]

[REVIEW NOTIFICATION]

This pull request has been approved by:

• AstroProfundis

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

[LittleFall]

/cc @tisonkun

[tisonkun]

Rest LGTM.

[AstroProfundis]

I think it's better figure out why

tiup/components/playground/main.go

Line 367 in 9a1ad59

case withMonitor:

is not working in this scenario and fix it.

[LittleFall]

hi @tisonkun @AstroProfundis, I've investigated why --monitor false is useless, and the conclusion is:

rootCmd.Flags().Bool(withMonitor, false, "Start prometheus and grafana component")

will create a pflag.Flag with NoOptDefVal is true. here is the code:

flag.NoOptDefVal = "true"

and when pflag parsing out command arguments, it won't accept pattern --flag arg when flag has NoOptDefVal. here is the code:

	if len(split) == 2 {
		// '--flag=arg'
		value = split[1]
	} else if flag.NoOptDefVal != "" {
		// '--flag' (arg was optional)
		value = flag.NoOptDefVal
	} else if len(a) > 0 {
		// '--flag arg'
		value = a[0]
		a = a[1:]
	} else {
		// '--flag' (arg was required)
		err = f.failf("flag needs an argument: %s", s)
		return
	}

[LittleFall]

A simple fix is set NoOptDefVal of flag Monitor to empty like this:

rootCmd.Flags().Bool(withMonitor, false, "Start prometheus and grafana component")
rootCmd.Flags().Lookup(withMonitor).NoOptDefVal = ""

but this will introduce a compatible issue:

	Before patch (NoOptDefVal="true")	After patch (NoOptDefVal="")
(Default)	enable monitor	enable monitor
--monitor=true	enable monitor	enable monitor
--monitor true	enable monitor	enable monitor
--monitor=false	Disable monitor	Disable monitor
--monitor false	enable monitor	Disable monitor
--monitor	enable monitor	Error: flag needs an argument: --monitor

what do you think about this?

[codecov-commenter]

Codecov Report

Merging #1512 (35d2814) into master (e59979e) will increase coverage by 4.54%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #1512      +/-   ##
==========================================
+ Coverage   26.27%   30.82%   +4.54%     
==========================================
  Files         267      279      +12     
  Lines       24041    24536     +495     
==========================================
+ Hits         6317     7562    +1245     
+ Misses      16901    15976     -925     
- Partials      823      998     +175     

Flag	Coverage Δ
integrate	21.86% <33.33%> (+5.82%)	⬆️
playground	13.73% <33.33%> (?)
tiup	16.04% <ø> (ø)
unittest	22.83% <0.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
components/playground/main.go	46.25% <33.33%> (+43.84%)	⬆️
...onents/playground/instance/tiflash_proxy_config.go	83.33% <0.00%> (ø)
components/playground/instance/tidb.go	83.78% <0.00%> (ø)
components/playground/instance/ticdc.go	0.00% <0.00%> (ø)
components/playground/instance/pump.go	0.00% <0.00%> (ø)
components/playground/instance/process.go	84.90% <0.00%> (ø)
components/playground/instance/pd.go	75.00% <0.00%> (ø)
components/playground/instance/tikv_config.go	100.00% <0.00%> (ø)
components/playground/instance/tiflash.go	61.98% <0.00%> (ø)
components/playground/instance/tikv.go	67.85% <0.00%> (ø)
... and 21 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e59979e...35d2814. Read the comment docs.

[LittleFall]

another thing is that after this patch, the help message will be like
--monitor Whether to start prometheus and grafana component (default true)
we can't make it to be
--monitor bool Whether to start prometheus and grafana component (default true)

because it's written in pflag code here

	name = flag.Value.Type()
	switch name {
	case "bool":
		name = ""
	case "float64":
		name = "float"
	case "int64":
		name = "int"
	case "uint64":
		name = "uint"
	case "stringSlice":
		name = "strings"
	case "intSlice":
		name = "ints"
	case "uintSlice":
		name = "uints"
	case "boolSlice":
		name = "bools"
	}

[tisonkun]

I think it is as complex as before...

[AstroProfundis]

Ummmmmm, I'd agree that change this argument to --disable-monitor/--without-monitor/--no-monitor and set it default to true might be easier.

[tisonkun]

I'd prefer disable- or without-.

BTW, what about user facing interface changes? Previous settings based on --monitor will break.

[AstroProfundis]

/merge

[ti-chi-bot]

This pull request has been accepted and is ready to merge.

Commit hash: cb4257b

[AstroProfundis]

/merge

[ti-chi-bot]

This pull request has been accepted and is ready to merge.

Commit hash: 4b9381c