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

[feature] support configuring database caches #1246

Conversation

NyaaaWhatsUpDoc
Copy link
Member

@NyaaaWhatsUpDoc NyaaaWhatsUpDoc commented Dec 9, 2022

Description

  • adds CacheConfiguration{} options to main configuration struct, allowing setting via config file
  • update config helpers generator to support nested structs
  • add example cache config variables to config tests

Checklist

  • I/we have read the GoToSocial contribution guidelines.
  • I/we have discussed the proposed changes already, either in an issue on the repository, or in the Matrix chat.
  • I/we have performed a self-review of added code.
  • I/we have written code that is legible and maintainable by others.
  • I/we have commented the added code, particularly in hard-to-understand areas.
  • I/we have made any necessary changes to documentation.
  • I/we have added tests that cover new code.
  • I/we have run tests and they pass locally with the changes.
  • I/we have run go fmt ./... and golangci-lint run.

…ation options

Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
@NyaaaWhatsUpDoc
Copy link
Member Author

NyaaaWhatsUpDoc commented Dec 9, 2022

Only thing still remaining is to update example configuration file, and possibly update other areas of documentation? And tests clearly...

Signed-off-by: kim <grufwub@gmail.com>
…e config docs to example config

Signed-off-by: kim <grufwub@gmail.com>
@NyaaaWhatsUpDoc
Copy link
Member Author

In time I want to add intelligent default for cache sizes.

@NyaaaWhatsUpDoc NyaaaWhatsUpDoc marked this pull request as ready for review December 10, 2022 10:50
@NyaaaWhatsUpDoc
Copy link
Member Author

This lays the groundwork for any changes (if we decide to make them) in #1244

@tsmethurst
Copy link
Contributor

Nice one kim! this looks really good :)

We gotta remember when we cut a release including these changes to include a migration note about the config format changing to allow nesting.

Also, do you foresee any issues with the nesting? I mean, could it break if one provides a config that doesn't nest?

@NyaaaWhatsUpDoc
Copy link
Member Author

Nice one kim! this looks really good :)

We gotta remember when we cut a release including these changes to include a migration note about the config format changing to allow nesting.

Also, do you foresee any issues with the nesting? I mean, could it break if one provides a config that doesn't nest?

for this specific PR there shouldn't be any breaking changes. the only nested values are new ones so shouldn't be an issue. for later PRs though -- I added a note about this in the issue #1244 :)

Copy link
Contributor

@tsmethurst tsmethurst left a comment

Choose a reason for hiding this comment

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

Great :) Just the one comment. Feel free to merge this after you've addressed it!

internal/config/state.go Show resolved Hide resolved
@tsmethurst tsmethurst added the ready Ready to merge when appropriate label Dec 11, 2022
@NyaaaWhatsUpDoc NyaaaWhatsUpDoc merged commit cb2b2fd into superseriousbusiness:main Dec 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready Ready to merge when appropriate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants