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

Add k/v store for repo settings #22500

Closed
wants to merge 17 commits into from
Closed

Conversation

lunny
Copy link
Member

@lunny lunny commented Jan 18, 2023

replace #19400
credit @techknowlogick

@lunny lunny added the type/enhancement An improvement of existing functionality label Jan 18, 2023
@lunny lunny added this to the 1.19.0 milestone Jan 18, 2023
models/migrations/v1_19/v240.go Outdated Show resolved Hide resolved
models/repo/setting.go Outdated Show resolved Hide resolved
models/repo/setting_keys.go Outdated Show resolved Hide resolved
models/repo/setting.go Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 18, 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 Jan 19, 2023
models/repo/setting.go Show resolved Hide resolved
models/repo/setting.go Outdated Show resolved Hide resolved
models/repo/setting.go Outdated Show resolved Hide resolved
models/repo/setting.go Outdated Show resolved Hide resolved
@lunny
Copy link
Member Author

lunny commented Feb 6, 2023

@delvh I think I did what you want. Please review again.

@lunny lunny changed the title add k/v store for repo settings Add k/v store for repo settings Feb 6, 2023
@yardenshoham yardenshoham modified the milestones: 1.19.0, 1.20.0 Feb 22, 2023
@yardenshoham yardenshoham added the outdated/backport/v1.19 This PR should be backported to Gitea 1.19 label Feb 22, 2023
models/db/setting.go Outdated Show resolved Hide resolved
models/migrations/v1_19/v242.go Outdated Show resolved Hide resolved
models/migrations/v1_20/v245.go Outdated Show resolved Hide resolved
models/repo/setting.go Outdated Show resolved Hide resolved
models/repo/setting.go Outdated Show resolved Hide resolved
models/db/setting.go Outdated Show resolved Hide resolved
models/db/setting.go Outdated Show resolved Hide resolved
models/db/setting.go Outdated Show resolved Hide resolved
models/db/setting.go Outdated Show resolved Hide resolved
models/db/setting.go Outdated Show resolved Hide resolved
@KN4CK3R
Copy link
Member

KN4CK3R commented Mar 9, 2023

Before we heavily start using our new user/repo/system setting tables, we should evaluate if the current database design is suitable. Currently we use something like

| reference id (repo.id/user.id int) | setting key (string) | setting value (string) |

If we add a setting with the key feature.subfeature.id for a user we need to store this string for every user we have. This could result in big pile of (unneeded) data. Sadly I did the same for the properties in the package registry, so I would call this design naïve.

This stackoverflow thread shows different ideas of how a settings system may be better designed. Additionally I would vote for different value type columns. At the moment we encode everything into strings. It may be beneficial to store values with the correct type: int(/bool) -> int, string/json -> string, ...

@KN4CK3R KN4CK3R removed the outdated/backport/v1.19 This PR should be backported to Gitea 1.19 label Mar 9, 2023
@lunny
Copy link
Member Author

lunny commented Mar 21, 2023

I have tried that design. But from another view of a database maintainer, it's not easy to find the value if we know the setting key and change it in the database directly. Maybe it's a smart database's responsibility to reduce storage space for duplicated keys.

@delvh done.

@wxiaoguang
Copy link
Contributor

I can see that repo has "unit" config, then why do we need this?

@lunny
Copy link
Member Author

lunny commented May 1, 2023

I can see that repo has "unit" config, then why do we need this?

Not all configuration belongs to a unit?

@wxiaoguang
Copy link
Contributor

I can see that repo has "unit" config, then why do we need this?

Not all configuration belongs to a unit?

For example?

@delvh delvh removed this from the 1.20.0 milestone Jun 7, 2023
@lunny
Copy link
Member Author

lunny commented Jul 5, 2023

Closed as currently most repository level configuration could be stored in repo_unit table.

@lunny lunny closed this Jul 5, 2023
@lunny lunny deleted the lunny/repo_settings branch July 5, 2023 08:11
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Oct 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants