Skip to content

Conversation

wxiaoguang
Copy link
Contributor

In #25959 I forgot to add default values to some Bool flags (which were BoolT in cli/v1, BoolT means default to be true)

This PR adds the default "Value" for them.

./cmd/manager_logging.go:							}, cli.BoolTFlag{
./cmd/manager_logging.go-								Name:  "rotate, r",
./cmd/manager_logging.go-								Usage: "Rotate logs",
--
./cmd/manager_logging.go:							}, cli.BoolTFlag{
./cmd/manager_logging.go-								Name:  "daily, d",
./cmd/manager_logging.go-								Usage: "Rotate logs daily",
--
./cmd/manager_logging.go:							}, cli.BoolTFlag{
./cmd/manager_logging.go-								Name:  "compress, z",
./cmd/manager_logging.go-								Usage: "Compress rotated logs",
--
./cmd/admin.go:		cli.BoolTFlag{
./cmd/admin.go-			Name:  "force-smtps",
./cmd/admin.go-			Usage: "SMTPS is always used on port 465. Set this to force SMTPS on other ports.",
--
./cmd/admin.go:		cli.BoolTFlag{
./cmd/admin.go-			Name:  "skip-verify",
./cmd/admin.go-			Usage: "Skip TLS verify.",
--
./cmd/admin.go:		cli.BoolTFlag{
./cmd/admin.go-			Name:  "disable-helo",
./cmd/admin.go-			Usage: "Disable SMTP helo.",
--
./cmd/admin.go:		cli.BoolTFlag{
./cmd/admin.go-			Name:  "skip-local-2fa",
./cmd/admin.go-			Usage: "Skip 2FA to log on.",
--
./cmd/admin.go:		cli.BoolTFlag{
./cmd/admin.go-			Name:  "active",
./cmd/admin.go-			Usage: "This Authentication Source is Activated.",

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 24, 2023
@lunny lunny added type/bug skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Jul 24, 2023
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 24, 2023
@lunny lunny added this to the 1.21.0 milestone Jul 24, 2023
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 24, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 24, 2023
@lunny lunny enabled auto-merge (squash) July 24, 2023 05:51
@lunny lunny merged commit 478f36a into go-gitea:main Jul 24, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 24, 2023
@wxiaoguang wxiaoguang deleted the fix-cli-bool branch July 24, 2023 06:52
@delvh
Copy link
Member

delvh commented Jul 24, 2023

#25959 (comment) 😁

@wxiaoguang
Copy link
Contributor Author

#25959 (comment) 😁

Yup, good catch, but I didn't realize that at that time. I vaguely feel that BoolTFlag is not the same as BoolFlag, but can't get out from the wrong way. 😭

zjjhot added a commit to zjjhot/gitea that referenced this pull request Jul 25, 2023
* giteaofficial/main: (23 commits)
  Avoid writing config file if not installed (go-gitea#26107)
  Implement auto-cancellation of concurrent jobs if the event is push (go-gitea#25716)
  [skip ci] Updated translations via Crowdin
  doc guide the user to create the appropriate level runner (go-gitea#26091)
  Fix handling of Debian files with trailing slash (go-gitea#26087)
  fix Missing 404 swagger response docs for /admin/users/{username} (go-gitea#26086)
  Allow the use of alternative net.Listener implementations by downstreams (go-gitea#25855)
  Add missing default value for some Bool cli flags (go-gitea#26082)
  Reduce unnecessary DB queries for Actions tasks (go-gitea#25199)
  Use stderr as fallback if the log file can't be opened (go-gitea#26074)
  Make organization redirect warning more clear (go-gitea#26077)
  Replace gogs/cron with go-co-op/gocron (go-gitea#25977)
  Remove `db.DefaultContext` in `routers/` and `cmd/` (go-gitea#26076)
  Categorize admin settings sidebar panel (go-gitea#26030)
  [skip ci] Updated translations via Crowdin
  Fix duplicated url prefix on issue context menu (go-gitea#26066)
  Add context parameter to some database functions (go-gitea#26055)
  Fix branch list auth (go-gitea#26041)
  Fix the truncate and alignment problem for some admin tables (go-gitea#26042)
  Update secrets.en-us.md (go-gitea#26057)
  ...
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Oct 22, 2023
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Jun 18, 2025

Thanks to @TheFox0x7 , it seems that many all changes (default: true) are wrong here. Need to revert them.

-> Fix incorrect cli default values #34765

The problem is that old code is

if c.IsSet("skip-local-2fa") {
    conf.SkipLocalTwoFA = c.BoolT("skip-local-2fa")
}

So it defaults to false.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants