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

Jump to bottom

Fix settings not being loaded at CLI #26402

Merged
merged 4 commits into from
Dec 30, 2024
Merged

Fix settings not being loaded at CLI #26402

merged 4 commits into from
Dec 30, 2024

Conversation

cassiozareck
Copy link
Contributor

@cassiozareck cassiozareck commented Aug 8, 2023 •
edited
Loading

Closes #25898
The problem was that the default settings weren't being loaded, so

gitea/models/user/user.go

Line 593 in ad69f71

u.AllowCreateOrganization = setting.Service.DefaultAllowCreateOrganization && !setting.Admin.DisableRegularOrgCreation

was always false.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 8, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 8, 2023
@techknowlogick techknowlogick added type/bug backport/v1.20 This PR should be backported to Gitea 1.20 labels Aug 8, 2023
@silverwind
Copy link
Member

silverwind commented Aug 11, 2023 •
edited
Loading

I wonder if this is really the right place to load the settings or if we should do it earlier. Doing it in the middle of command processing does seem odd.

@cassiozareck
Copy link
Contributor Author

Both way works as of my tests. I thought to initialize together with the DB but thinking better at the beginning may be more clean. I'll adjust it

@cassiozareck
Fix settings not being loaded at CLI
6096745 
Signed-off-by: cassiozareck <cassiomilczareck@gmail.com>
@silverwind
Copy link
Member

Ah I see the context now, previous place was fine then I guess, the validation does not depend on settings it looks like. Note I'm not really knowledgeable around these parts so take my comments with a grain of salt :)

@cassiozareck
Copy link
Contributor Author

cassiozareck commented Aug 11, 2023 •
edited
Loading

As we're loading settings there's happening some output when a user runs gitea admin user create. Like this:

2023/08/11 10:34:13 ...les/setting/cache.go:75:loadCacheFrom() [I] Cache Service Enabled
2023/08/11 10:34:13 ...les/setting/cache.go:90:loadCacheFrom() [I] Last Commit Cache Service Enabled
2023/08/11 10:34:13 ...s/setting/session.go:74:loadSessionFrom() [I] Session Service Enabled
New user 'user1' has been successfully created!

@silverwind
Copy link
Member

That reinforces that it needs to be after validation.

@cassiozareck
Copy link
Contributor Author

Hmm so its ok? I think its unavoidable unless we try to change the config for logging outputs before

@silverwind
Copy link
Member

Just revert to previous location imho.

@cassiozareck
Copy link
Contributor Author

cassiozareck commented Aug 12, 2023 •
edited
Loading

Oh no no! It happen doesn't matter where loadsetting is. There's log statements inside loadsettings. I think its alright as it states settings are being loaded but if you dont want log to happen I can see what I can do. I know I caused some confusion because it looked like I was saying because of position. Pls dont avoid my PRs 😆

@lunny lunny removed the backport/v1.20 This PR should be backported to Gitea 1.20 label Dec 25, 2024
@lunny lunny added the backport/v1.23 This PR should be backported to Gitea 1.23 label Dec 28, 2024
@lunny lunny added this to the 1.24.0 milestone Dec 28, 2024
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Dec 29, 2024 •
edited
Loading

This fix works but it is somewhat hacky and fragile.

Ideally, there should be command line options to set the flags, but not continue loading more unclear settings.

There is already overwrite.AllowCreateOrganization, the same to Visibility

lunny
lunny approved these changes Dec 30, 2024
View reviewed changes
Copy link
Member

@lunny lunny left a comment

Choose a reason for hiding this comment

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

As a quick fix, I think it's enough.

@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 Dec 30, 2024
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin labels Dec 30, 2024
@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 30, 2024
@wxiaoguang
Copy link
Contributor

Made some changes in 170261a :

  • do not show unnecessary logs
  • add comments

@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 Dec 30, 2024
@wxiaoguang wxiaoguang enabled auto-merge (squash) December 30, 2024 05:30
@wxiaoguang wxiaoguang merged commit 1e2c8eb into go-gitea:main Dec 30, 2024
26 checks passed
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Dec 30, 2024
@cassiozareck @lunny
@wxiaoguang @GiteaBot
Fix settings not being loaded at CLI (go-gitea#26402)
9143439 
Closes go-gitea#25898
The problem was that the default settings weren't being loaded

---------

Signed-off-by: cassiozareck <cassiomilczareck@gmail.com>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@GiteaBot GiteaBot mentioned this pull request Dec 30, 2024
Merged
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Dec 30, 2024
wxiaoguang added a commit that referenced this pull request Dec 30, 2024
@GiteaBot @cassiozareck
@lunny @wxiaoguang
Fix settings not being loaded at CLI (#26402) (#33048)
27de603 
Backport #26402 by cassiozareck

Closes #25898

Signed-off-by: cassiozareck <cassiomilczareck@gmail.com>
Co-authored-by: cassio zareck <121526696+cassiozareck@users.noreply.github.com>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Dec 31, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
8aaec8e 
* giteaofficial/main:
  Fix issue comment number (go-gitea#30556)
  Fix duplicate co-author in squashed merge commit messages (go-gitea#33020)
  Merge updatecommentattachment functions (go-gitea#33044)
  Move SetMerged to service layer (go-gitea#33045)
  Remove aws go sdk package dependency (go-gitea#33029)
  Fix settings not being loaded at CLI (go-gitea#26402)
  Refactor fixture loading for testing (go-gitea#33024)
  Use gitrepo.GetTreePathLatestCommit to get file lastest commit instead from latest commit cache (go-gitea#32987)
  Fix bug automerge cannot be chosed when there is only 1 merge style (go-gitea#33040)
  use `-s -w` ldflags for release artifacts (go-gitea#33041)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created backport/v1.23 This PR should be backported to Gitea 1.23 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/go Pull requests that update Go code size/S Denotes a PR that changes 10-29 lines, ignoring generated files. type/bug
Projects
None yet
Milestone
1.24.0
Development

Successfully merging this pull request may close these issues.

Create org permissions of user created via cli client
6 participants
@cassiozareck @silverwind @wxiaoguang @lunny @techknowlogick @GiteaBot