Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update documentation to disable duration settings with -1 instead of 0 #19647

Merged
merged 6 commits into from
May 9, 2022

Conversation

jpraet
Copy link
Member

@jpraet jpraet commented May 7, 2022

To turn off the notification endpoint polling, the value should be set to -1, not 0.

To turn off the notification endpoint polling, the value should be set to -1, not 0.
@jpraet jpraet added the type/docs This PR mainly updates/creates documentation label May 7, 2022
@delvh delvh added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label May 7, 2022
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label May 7, 2022
@codecov-commenter

This comment was marked as off-topic.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 7, 2022

-1 and 0 both should work. it's not breaking. and how to document it clearly? recommend to always use 0 from now on?

// in go code
"MinTimeout": int(setting.UI.Notification.MinTimeout / time.Millisecond)
  // and in JS code
  if (notificationSettings.MinTimeout <= 0) {
    return;
  }

@delvh
Copy link
Member

delvh commented May 7, 2022

Yes, if 0 works as well, it indeed is not breaking.
I've assumed it wasn't, otherwise this PR does not make much sense.

@6543 6543 added this to the 1.17.0 milestone May 7, 2022
@delvh delvh removed the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label May 7, 2022
@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 May 7, 2022
@jpraet
Copy link
Member Author

jpraet commented May 7, 2022

0 does not work. I've put a console log of notificationSettings.MinTimeout in the JS code and it prints 10000 when MIN_TIMEOUT is set to 0. When MIN_TIMEOUT is set to -1, it prints 0.

Note that setting.UI.Notification.MinTimeout is of type time.Duration, I'm not sure where to find the conversion code from string to time.Duration?

@jpraet
Copy link
Member Author

jpraet commented May 7, 2022

What about these? I think it's likely these duration properties may also not work as documented?

- `PER_WRITE_TIMEOUT`: **30s**: Timeout for any write to the connection. (Set to 0 to
disable all timeouts.)

- `SSH_PER_WRITE_TIMEOUT`: **30s**: Timeout for any write to the SSH connections. (Set to
0 to disable all timeouts.)

- `STARTUP_TIMEOUT`: **30s**: If the indexer takes longer than this timeout to start - fail. (This timeout will be added to the hammer time above for child processes - as bleve will not start until the previous parent is shutdown.) Set to zero to never timeout.

- `ITEM_TTL`: **16h**: Time to keep items in cache if not used, Setting it to 0 disables caching.

- `ITEM_TTL`: **8760h**: Time to keep items in cache if not used, Setting it to 0 disables caching.

Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we should look into the problem. IMO 0 is a general value for defaults.

I will take a look at it, just put a request change mark here temporarily

@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 7, 2022

It's a bug in the legacy ini package (Hmm, not legacy, still active)

Then, at the moment, all time.Duration fields in setting can not accept option value 0 if there is already a non-zero default value.

I would suggest to fix it from upstream, instead of tolerate and bypass the problem .........

	// around ini/struct.go:181
	case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
		// ParseDuration will not return err for `0`, so check the type name
		if vt.Name() == "Duration" {
			durationVal, err := key.Duration()
			if err != nil {
				if intVal, err := key.Int64(); err == nil {
					field.SetInt(intVal)
					return nil
				}
				return wrapStrictError(err, isStrict)
			}
			if isPtr {
				field.Set(reflect.ValueOf(&durationVal))
			} else if int64(durationVal) > 0 {  // 🐞 BUG HERE, even the key/value(0) exists, the ini code ignore the value
				field.Set(reflect.ValueOf(durationVal))
			}
			return nil
		}

@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 7, 2022

After reading the ini code again, I do not have confidence that such behavior could be fixed soon if the ini author doesn't agree to change, the fix might cause some breaking behaviors.

So, at the moment, maybe all document should be updated accordingly, use -1 for all no-duration values.

@jpraet jpraet changed the title Fix ui.notification MIN_TIMEOUT documentation Update documentation to disable duration settings with -1 instead of 0 May 7, 2022
@delvh
Copy link
Member

delvh commented May 7, 2022

@wxiaoguang That sounds to me as if this PR should indeed be marked breaking.
Otherwise users might not know to change their configurations.

@wxiaoguang
Copy link
Contributor

IMO there is no code change, it doesn't break anything, it's a document bug fix only. And the fixes also works for 1.16.

If users do not find this bug (not affected), they do not need to change their configuration ......

@6543
Copy link
Member

6543 commented May 9, 2022

🚀

@6543 6543 merged commit a61a47f into go-gitea:main May 9, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request May 10, 2022
* giteaofficial/main:
  Use better message for consistency check (go-gitea#19672)
  Fix new release from tags list UI (go-gitea#19670)
  Update go deps (go-gitea#19665)
  [doctor] Add check/fix for bogus action rows (go-gitea#19656)
  [skip ci] Updated translations via Crowdin
  Add tooltip to pending PR comments (go-gitea#19662)
  Add Webfinger endpoint (go-gitea#19462)
  Update documentation to disable duration settings with -1 instead of 0 (go-gitea#19647)
  Set the LastModified header for raw files (go-gitea#18356)
  Don't select join table's columns (go-gitea#19660)
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
go-gitea#19647)

To turn off the notification endpoint polling, the value should be set to -1, not 0.
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
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. type/docs This PR mainly updates/creates documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants