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.

Sign up for GitHub

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

Notify about outdated settings #23203

Closed
wants to merge 11 commits into from
Closed

Notify about outdated settings #23203

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d07784e
Add advanced mechanism to notify about removed/deprecated settings
delvh Feb 5, 2023
93bbad1
Rename methods, add previously removed settings, better tense detection
delvh Feb 11, 2023
008f77d
Refactor setting history to use composition instead
delvh Feb 18, 2023
aeb965d
Refactor mechanism completely once more
delvh Feb 25, 2023
c45ab0c
Extract the changes into separate files
delvh Feb 25, 2023
7f5503b
rename functions and store which types of source an event belongs to
delvh Feb 28, 2023
f3ddbd1
Merge remote-tracking branch 'origin/main' into feature/notify-about-…
delvh Feb 28, 2023
61e8af2
Remove previous `deprecatedSetting` methods with the new mechanism
delvh Feb 28, 2023
9f24391
Fix final bugs, move config provider into its own package
delvh Feb 28, 2023
b25df8b
Fix bug printing unset outdated settings as set and use maps as provider
delvh Mar 1, 2023
78dc2d0
Run `make fmt`
delvh Mar 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions cmd/web.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
package cmd

import (
"code.gitea.io/gitea/modules/setting/base"
"code.gitea.io/gitea/modules/setting/history"
"context"
"fmt"
"net"
Expand Down Expand Up @@ -157,8 +159,13 @@ func runWeb(ctx *cli.Context) error {
}

log.Info("Global init")

// Perform global initialization
setting.InitProviderFromExistingFile()

// Print now outdated settings - is before any other setting action to prevent accidental initialization of other keys
history.PrintRemovedSettings(map[history.SettingsSource]base.ConfigProvider{history.SettingsSourceINI: setting.CfgProvider}) // TODO: modules/setting/setting.go#loadCommonSettingsFrom would be more fitting as a place for this call, but it is called twice during initialization for some reason

setting.LoadCommonSettings()
routers.GlobalInitInstalled(graceful.GetManager().HammerContext())

Expand Down Expand Up @@ -217,6 +224,7 @@ func listen(m http.Handler, handleRedirector bool) error {
}
_, _, finished := process.GetManager().AddTypedContext(graceful.GetManager().HammerContext(), "Web: Gitea Server", process.SystemProcessType, true)
defer finished()

log.Info("Listen: %v://%s%s", setting.Protocol, listenAddr, setting.AppSubURL)
// This can be useful for users, many users do wrong to their config and get strange behaviors behind a reverse-proxy.
// A user may fix the configuration mistake when he sees this log.
Expand Down
3 changes: 2 additions & 1 deletion modules/setting/actions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package setting

import (
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting/base"
)

// Actions settings
Expand All @@ -19,7 +20,7 @@ var (
}
)

func loadActionsFrom(rootCfg ConfigProvider) {
func loadActionsFrom(rootCfg base.ConfigProvider) {
sec := rootCfg.Section("actions")
if err := sec.MapTo(&Actions); err != nil {
log.Fatal("Failed to map Actions settings: %v", err)
Expand Down
6 changes: 4 additions & 2 deletions modules/setting/admin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,16 @@

package setting

import "code.gitea.io/gitea/modules/setting/base"

// Admin settings
var Admin struct {
DisableRegularOrgCreation bool
DefaultEmailNotification string
}

func loadAdminFrom(rootCfg ConfigProvider) {
mustMapSetting(rootCfg, "admin", &Admin)
func loadAdminFrom(rootCfg base.ConfigProvider) {
base.MustMapSetting(rootCfg, "admin", &Admin)
sec := rootCfg.Section("admin")
Admin.DefaultEmailNotification = sec.Key("DEFAULT_EMAIL_NOTIFICATIONS").MustString("enabled")
}
5 changes: 3 additions & 2 deletions modules/setting/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"path"

"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting/base"
)

// API settings
Expand All @@ -27,8 +28,8 @@ var API = struct {
DefaultMaxBlobSize: 10485760,
}

func loadAPIFrom(rootCfg ConfigProvider) {
mustMapSetting(rootCfg, "api", &API)
func loadAPIFrom(rootCfg base.ConfigProvider) {
base.MustMapSetting(rootCfg, "api", &API)

defaultAppURL := string(Protocol) + "://" + Domain + ":" + HTTPPort
u, err := url.Parse(rootCfg.Section("server").Key("ROOT_URL").MustString(defaultAppURL))
Expand Down
4 changes: 3 additions & 1 deletion modules/setting/attachment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

package setting

import "code.gitea.io/gitea/modules/setting/base"

// Attachment settings
var Attachment = struct {
Storage
Expand All @@ -20,7 +22,7 @@ var Attachment = struct {
Enabled: true,
}

func loadAttachmentFrom(rootCfg ConfigProvider) {
func loadAttachmentFrom(rootCfg base.ConfigProvider) {
sec := rootCfg.Section("attachment")
storageType := sec.Key("STORAGE_TYPE").MustString("")

Expand Down
27 changes: 27 additions & 0 deletions modules/setting/base/config_provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// Copyright 2023 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT

package base

import (
"code.gitea.io/gitea/modules/log"

ini "gopkg.in/ini.v1"
)

// ConfigProvider represents a config provider
type ConfigProvider interface {
Section(section string) *ini.Section
NewSection(name string) (*ini.Section, error)
GetSection(name string) (*ini.Section, error)
HasSection(name string) bool
}

// a file is an implementation ConfigProvider and other implementations are possible, i.e. from docker, k8s, …
var _ ConfigProvider = &ini.File{}

func MustMapSetting(rootCfg ConfigProvider, sectionName string, setting interface{}) {
if err := rootCfg.Section(sectionName).MapTo(setting); err != nil {
log.Fatal("Failed to map %s settings: %v", sectionName, err)
}
}
3 changes: 2 additions & 1 deletion modules/setting/cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"time"

"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting/base"
)

// Cache represents cache settings
Expand Down Expand Up @@ -49,7 +50,7 @@ var CacheService = struct {
// MemcacheMaxTTL represents the maximum memcache TTL
const MemcacheMaxTTL = 30 * 24 * time.Hour

func loadCacheFrom(rootCfg ConfigProvider) {
func loadCacheFrom(rootCfg base.ConfigProvider) {
sec := rootCfg.Section("cache")
if err := sec.MapTo(&CacheService); err != nil {
log.Fatal("Failed to map Cache settings: %v", err)
Expand Down
9 changes: 6 additions & 3 deletions modules/setting/camo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@

package setting

import "code.gitea.io/gitea/modules/log"
import (
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting/base"
)

var Camo = struct {
Enabled bool
Expand All @@ -12,8 +15,8 @@ var Camo = struct {
Allways bool
}{}

func loadCamoFrom(rootCfg ConfigProvider) {
mustMapSetting(rootCfg, "camo", &Camo)
func loadCamoFrom(rootCfg base.ConfigProvider) {
base.MustMapSetting(rootCfg, "camo", &Camo)
if Camo.Enabled {
if Camo.ServerURL == "" || Camo.HMACKey == "" {
log.Fatal(`Camo settings require "SERVER_URL" and HMAC_KEY`)
Expand Down
39 changes: 0 additions & 39 deletions modules/setting/config_provider.go
View file
Open in desktop

This file was deleted.

5 changes: 3 additions & 2 deletions modules/setting/cors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"time"

"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting/base"
)

// CORSConfig defines CORS settings
Expand All @@ -27,8 +28,8 @@ var CORSConfig = struct {
XFrameOptions: "SAMEORIGIN",
}

func loadCorsFrom(rootCfg ConfigProvider) {
mustMapSetting(rootCfg, "cors", &CORSConfig)
func loadCorsFrom(rootCfg base.ConfigProvider) {
base.MustMapSetting(rootCfg, "cors", &CORSConfig)
if CORSConfig.Enabled {
log.Info("CORS Service Enabled")
}
Expand Down
8 changes: 6 additions & 2 deletions modules/setting/cron.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,18 @@

package setting

import "reflect"
import (
"reflect"

"code.gitea.io/gitea/modules/setting/base"
)

// GetCronSettings maps the cron subsection to the provided config
func GetCronSettings(name string, config interface{}) (interface{}, error) {
return getCronSettings(CfgProvider, name, config)
}

func getCronSettings(rootCfg ConfigProvider, name string, config interface{}) (interface{}, error) {
func getCronSettings(rootCfg base.ConfigProvider, name string, config interface{}) (interface{}, error) {
if err := rootCfg.Section("cron." + name).MapTo(config); err != nil {
return config, err
}
Expand Down
3 changes: 2 additions & 1 deletion modules/setting/federation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package setting

import (
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting/base"

"github.com/go-fed/httpsig"
)
Expand Down Expand Up @@ -33,7 +34,7 @@ var (
// HttpsigAlgs is a constant slice of httpsig algorithm objects
var HttpsigAlgs []httpsig.Algorithm

func loadFederationFrom(rootCfg ConfigProvider) {
func loadFederationFrom(rootCfg base.ConfigProvider) {
if err := rootCfg.Section("federation").MapTo(&Federation); err != nil {
log.Fatal("Failed to map Federation settings: %v", err)
} else if !httpsig.IsSupportedDigestAlgorithm(Federation.DigestAlgorithm) {
Expand Down
3 changes: 2 additions & 1 deletion modules/setting/git.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"time"

"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting/base"
)

// Git settings
Expand Down Expand Up @@ -67,7 +68,7 @@ var Git = struct {
},
}

func loadGitFrom(rootCfg ConfigProvider) {
func loadGitFrom(rootCfg base.ConfigProvider) {
sec := rootCfg.Section("git")
if err := sec.MapTo(&Git); err != nil {
log.Fatal("Failed to map Git settings: %v", err)
Expand Down
72 changes: 72 additions & 0 deletions modules/setting/history/breaking_changes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
// Copyright 2023 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT

package history

import "code.gitea.io/gitea/modules/log"

// This file is the only file that should be changed frequently in this package

var currentGiteaVersion = getVersion("1.19")
Copy link
Member

Choose a reason for hiding this comment

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

Wow, I don't think it's a good idea to ask maintainers to remember this.

Copy link
Contributor

Choose a reason for hiding this comment

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

In my mind, maybe:

  • Prod mode: parse version string
  • Dev mode: run git to describe the base tag to get the working version?

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you please elaborate on how I get the information on what our current version is?
If I know how to do it, I can implement it.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • Released binary: there is a var containing the build version (set by Makefile from git describe)
  • Dev mode: git describe:
~/work/gitea$ git describe
v1.20.0-dev-52-g151d3e7832

Copy link
Member

Choose a reason for hiding this comment

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

From #26094

The main problem of that PR is that I still haven't found out how I have to set LD_FLAGS so that the version will be updated automatically.

The version is currently set in main

gitea/main.go

Line 28 in 6aa30af

Version = "development" // program version for this build

gitea/Makefile

Line 108 in 6aa30af

LDFLAGS := $(LDFLAGS) -X "main.MakeVersion=$(MAKE_VERSION)" -X "main.Version=$(GITEA_VERSION)" -X "main.Tags=$(TAGS)"

To use it elsewhere you'll need to move it to a non-main package.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is especially that it isn't in the main package, and my attempts to convert it to modules.setting.history.currentGiteaVersion were all fruitless endeavors thus far.
That's why this PR has moved to the back of my TODO list as I haven't found out yet what to use.

Copy link
Member

Choose a reason for hiding this comment

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

-X code.gitea.io/gitea/modules/setting/history.currentGiteaVersion=$(GITEA_VERSION) should do the trick, although I'd recommend exporting the variable so we can use the same one in both places.


// Adds all previously removed settings
// It should declare all breaking configuration changes in chronological order to ensure a monotone increasing error log
func init() {
Copy link
Member

Choose a reason for hiding this comment

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

So these information will be displayed before setting.Init ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, init seems uncontrollable. Is the logging system ready at the moment?

Copy link
Member Author

@delvh delvh Mar 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is the logging system ready at the moment?

Yes, that's one of the reasons it is in its own package. (The other is a clear separation of what belongs to what)
The package is only loaded when PrintRemovedSettings is called, which is exactly after the logging has been set up, and before any other processing has happened.
If we decide to drop these two trace calls, it doesn't even matter for the init method if the other package has been loaded already or not, the log is then only needed for PrintRemovedSettings.
So, what is the preferred course of action here?
Dropping the trace calls, or keeping them in and relying on the logger being initialized?
Or replacing the init with an Init we call ourselves?

So these information will be displayed before setting.Init ?

No, it is called as soon as setting.CfgProvider has been set, but before any other settings have been initialized.
Since there is no setting.Init (anymore?), that's the best course of action.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or replacing the init with an Init we call ourselves?

I think we should make the Init called explicitly, it makes the calling order controllable.

log.Trace("Start checking settings for if they are outdated")
MoveIniSettingInSection("1.6", "api", "ENABLE_SWAGGER_ENDPOINT", "ENABLE_SWAGGER")

PurgeIniSettings("1.9", "log.database", "LEVEL", "DRIVER", "CONN")

MoveIniSetting("1.12", "markup.sanitizer", "ELEMENT", "markup.sanitizer.1", "ELEMENT")
MoveIniSetting("1.12", "markup.sanitizer", "ALLOW_ATTR", "markup.sanitizer.1", "ALLOW_ATTR")
MoveIniSetting("1.12", "markup.sanitizer", "REGEXP", "markup.sanitizer.1", "REGEXP")

PurgeIniSettings("1.14", "log", "MACARON", "REDIRECT_MACARON_LOG")

MoveIniSetting("1.15", "indexer", "ISSUE_INDEXER_QUEUE_TYPE", "queue.issue_indexer", "TYPE")
MoveIniSetting("1.15", "indexer", "ISSUE_INDEXER_QUEUE_DIR", "queue.issue_indexer", "DATADIR")
MoveIniSetting("1.15", "indexer", "ISSUE_INDEXER_QUEUE_CONN_STR", "queue.issue_indexer", "CONN_STR")
MoveIniSetting("1.15", "indexer", "ISSUE_INDEXER_QUEUE_BATCH_NUMBER", "queue.issue_indexer", "BATCH_LENGTH")
MoveIniSetting("1.15", "indexer", "UPDATE_BUFFER_LEN", "queue.issue_indexer", "LENGTH")

MoveIniSettingInSection("1.17", "cron.archive_cleanup", "NO_SUCCESS_NOTICE", "NOTIFY_ON_SUCCESS")
MoveIniSettingInSection("1.17", "cron.update_mirrors", "NO_SUCCESS_NOTICE", "NOTIFY_ON_SUCCESS")
MoveIniSettingInSection("1.17", "cron.repo_health_check", "NO_SUCCESS_NOTICE", "NOTIFY_ON_SUCCESS")
MoveIniSettingInSection("1.17", "cron.check_repo_stats", "NO_SUCCESS_NOTICE", "NOTIFY_ON_SUCCESS")
MoveIniSettingInSection("1.17", "cron.update_migration_poster_id", "NO_SUCCESS_NOTICE", "NOTIFY_ON_SUCCESS")
MoveIniSettingInSection("1.17", "cron.sync_external_users", "NO_SUCCESS_NOTICE", "NOTIFY_ON_SUCCESS")
MoveIniSettingInSection("1.17", "cron.deleted_branches_cleanup", "NO_SUCCESS_NOTICE", "NOTIFY_ON_SUCCESS")
MoveIniSettingInSection("1.17", "cron.delete_inactive_accounts", "NO_SUCCESS_NOTICE", "NOTIFY_ON_SUCCESS")
MoveIniSettingInSection("1.17", "cron.delete_repo_archives", "NO_SUCCESS_NOTICE", "NOTIFY_ON_SUCCESS")
MoveIniSettingInSection("1.17", "cron.git_gc_repos", "NO_SUCCESS_NOTICE", "NOTIFY_ON_SUCCESS")
MoveIniSettingInSection("1.17", "cron.resync_all_sshkeys", "NO_SUCCESS_NOTICE", "NOTIFY_ON_SUCCESS")
MoveIniSettingInSection("1.17", "cron.resync_all_hooks", "NO_SUCCESS_NOTICE", "NOTIFY_ON_SUCCESS")
MoveIniSettingInSection("1.17", "cron.reinit_missing_repos", "NO_SUCCESS_NOTICE", "NOTIFY_ON_SUCCESS")
MoveIniSettingInSection("1.17", "cron.delete_missing_repos", "NO_SUCCESS_NOTICE", "NOTIFY_ON_SUCCESS")
MoveIniSettingInSection("1.17", "cron.delete_generated_repository_avatars", "NO_SUCCESS_NOTICE", "NOTIFY_ON_SUCCESS")
MoveIniSettingInSection("1.17", "cron.delete_old_actions", "NO_SUCCESS_NOTICE", "NOTIFY_ON_SUCCESS")

PurgeIniSettings("1.18", "U2F", "APP_ID")
MoveIniSettingsToDB("1.18", "picture", "ENABLE_FEDERATED_AVATAR", "DISABLE_GRAVATAR")
MoveIniSettingInSection("1.18", "mailer", "HOST", "SMTP_ADDR")
MoveIniSettingInSection("1.18", "mailer", "MAILER_TYPE", "PROTOCOL")
MoveIniSettingInSection("1.18", "mailer", "IS_TLS_ENABLED", "PROTOCOL")
MoveIniSettingInSection("1.18", "mailer", "DISABLE_HELO", "ENABLE_HELO")
MoveIniSettingInSection("1.18", "mailer", "SKIP_VERIFY", "FORCE_TRUST_SERVER_CERT")
MoveIniSettingInSection("1.18", "mailer", "USE_CERTIFICATE", "USE_CLIENT_CERT")
MoveIniSettingInSection("1.18", "mailer", "CERT_FILE", "CLIENT_CERT_FILE")
MoveIniSettingInSection("1.18", "mailer", "KEY_FILE", "CLIENT_KEY_FILE")
MoveIniSettingInSection("1.18", "server", "ENABLE_LETSENCRYPT", "ENABLE_ACME")
MoveIniSettingInSection("1.18", "server", "LETSENCRYPT_ACCEPTTOS", "ACME_ACCEPTTOS")
MoveIniSettingInSection("1.18", "server", "LETSENCRYPT_DIRECTORY", "ACME_DIRECTORY")
MoveIniSettingInSection("1.18", "server", "LETSENCRYPT_EMAIL", "ACME_EMAIL")
MoveIniSetting("1.18", "server", "LFS_CONTENT_PATH", "lfs", "PATH")
MoveIniSetting("1.18", "task", "QUEUE_TYPE", "queue.task", "TYPE")
MoveIniSetting("1.18", "task", "QUEUE_CONN_STR", "queue.task", "CONN_STR")
MoveIniSetting("1.18", "task", "QUEUE_LENGTH", "queue.task", "LENGTH")
MoveIniSetting("1.18", "repository", "DISABLE_MIRRORS", "mirror", "ENABLED")

PurgeIniSettings("1.19", "ui", "ONLY_SHOW_RELEVANT_REPOS")

log.Trace("Finish checking settings for if they are outdated")
}
Loading