[bug] Discrepancies in default values in config.yaml and in the code

[firescry]

Describe the bug with a clear and concise description of what the bug is.

There are few discrepancies in default values of parameters defined in example/config.yaml and in internal/config/defaults.go.

Here is the list of all discrepancies I've found:

#	example/config.yaml	internal/config/defaults.go
1	absent	AdminMediaPruneDryRun: true
2	advanced-throttling-retry-after: "30s"	absent
3	block-max-size: 100	BlockMaxSize: 1000
4	absent but ok :)	ConfigPath: ""
5	db-sqlite-busy-timeout: "5m"	DbSqliteBusyTimeout: time.Minute * 30
6	host: "localhost"	Host: ""
7	instance-expose-public-timeline: false	absent
8	absent	InstanceMaxSize: 2000
9	absent	InstanceSweepFreq: time.Minute
10	absent	InstanceTTL: time.Minute * 30
11	oidc-admin-groups: []	absent
12	smtp-from: ""	SMTPFrom: "GoToSocial"
13	storage-s3-access-key: ""	absent
14	storage-s3-bucket: ""	absent
15	storage-s3-endpoint: ""	absent
16	storage-s3-secret-key: ""	absent
17	webfinger-sweep-freq: "1m"	WebfingerSweepFreq: time.Minute * 15

What's your GoToSocial Version?

v0.10.0-rc1

GoToSocial Arch

All are affected

What happened?

Application may act differently for different users if they chose to configure it with or without config.yaml (e.g. using only environment variables or runtime parameters to overwrite hard-coded defaults). This is in regards to items 2, 3, 5, 12 and 17.

Other items may result in rather small inconvenience (user would need to add them to config.yaml to change their default value) - items 1, 8, 9, 10 - or may be perceived as inconsistency in the code - items 7, 11, 13, 14, 15, 16 (e.g. there are other variables declared with empty string value).

What you expected to happen?

I think the default values should be the same no matter which way of configuration user chooses.

How to reproduce it?

No response

Anything else we need to know?

I would gladly work on providing a patch to address these discrepancies.
Please let me know if you see a value in such contribution and if you think that all or maybe only some of them should be aligned :)

[daenney]

Thanks for this! There's definitely a few we need to fix there.

In Go, when a value isn't explicitly initialised, you'll get the "zero value" of the type, or nil if it's a pointer. In our case, that means that if you don't explicitly set a value to a string you get the empty string "", for a boolean it'll be false and for a list it'll be the empty list, [].

This makes instance-expose-public-timeline, oidc-admin-groups and all the storage-s3-* keys equivalent. There's no need to explicitly set them in defaults.go. We typically only set things there where the default we want is not the same as the zero value of the type.

[daenney]

Having said that, I realise that there's a whole bunch of places where we do explicitly intialise values to false or "" etc. The effect ends up being the same, as the actual entries in config.go don't use pointer types. So we could either complete the list, or remove all the ones where the default matches the type. I would probably leave them as is though.

For the ones where we have a default in example/config.yaml that doesn't match the ones in default.go, I would suggest updating config.yaml to match. defaults.go is the authoritative one. Often happens if we've tweaked the values after the initial PR landed or if we changed some things along the way but forgot to update the docs.

[firescry]

Thank you for detailed response and guidelines 😊

I prepared changes according to our discussion - #1971.

I marked my merge request as draft for now as I'd like to propose one improvement - default values could be checked in automatic way as part of CI (similar to test/envparsing.sh). I created some basic script to perform such check on a branch in my fork, which you may see here - test/defaults.sh. The result of such check is here - https://github.com/firescry/gotosocial/actions/runs/5501514485/jobs/10025156307. (The script is not yet finished - for sure it needs to be able to skip on expected differences, e.g. config-path)

I think this check could help in keeping config.yaml values consistent with the code. Please let me know if you think that such check would be a good addition to current CI.

[igalic]

would it be possible to generate the go code defaults from the YAML at build time?

[daenney]

would it be possible to generate the go code defaults from the YAML at build time?

I've been thinking the same, but the other way around. I was hoping to generate example/config.yaml by loading the configuration with the defaults and then serialising it back out. That would ensure they're always up to date, and the code is the more likely one we're to change. That would hopefully also let us generate the CLI reference docs, which would be nice too as they're incomplete now.

The one thing I don't know is how/where to stash the additional prose we have in example/config.yaml so that it's not lost when it's regenerated.