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 contrib/ini-to-shell #32669

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

justusbunsi
Copy link
Member

Description of the change

This adds a new binary for extracting settings from an existing app.ini.

Benefits

Instead of grep-ing the file via shell scripts, this binary uses the Gitea internals for reading the app.ini.

Possible drawbacks

None

Applicable issues

Helps addressing https://gitea.com/gitea/helm-chart/issues/356.


Signed-off-by: justusbunsi [email protected]

Signed-off-by: justusbunsi <[email protected]>
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 28, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 28, 2024
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/internal labels Nov 28, 2024
@justusbunsi justusbunsi requested a review from a team November 28, 2024 19:33
@justusbunsi
Copy link
Member Author

Test error seems unrelated

Comment on lines +96 to +98
golog.SetOutput(os.Stdout)
golog.SetFlags(golog.Flags() &^ (golog.Ldate | golog.Ltime))
golog.Println(section.Key(kName).Value())
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think it should use log module, it should just print to stdout as-is

Comment on lines +73 to +77
setting.InitWorkPathAndCfgProvider(os.Getenv, setting.ArgWorkPathAndCustomConf{
WorkPath: c.String("work-path"),
CustomPath: c.String("custom-path"),
CustomConf: c.String("config"),
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Necessary?

I think we only need to pass the "config file" to the program, without InitWorkPathAndCfgProvider, and just NewConfigProviderFromFile

@wxiaoguang
Copy link
Contributor

https://gitea.com/gitea/helm-chart/issues/356
#32669 will help us extract all values that must be persistent.

I do not quite understand the usage. Could you provide more details about how to "extract all values that must be persistent" and "restore default"?

The ini config system is quite tricky, if something wrong, it wouldn't be easy to fix.

@KN4CK3R
Copy link
Member

KN4CK3R commented Nov 29, 2024

The problem is that some settings are passend by env to Gitea and these settings then are written to the ini file. If you remove the setting from the env the user expects to get the default value again. But that's not the case because the old value was written to the ini. To get rid of it they would like to create a new ini on startup but recreating the ini looses values like SECRET_KEY. This app will extract these values from the ini.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Nov 29, 2024

The problem is that some settings are passend by env to Gitea and these settings then are written to the ini file. If you remove the setting from the env the user expects to get the default value again. But that's not the case because the old value was written to the ini. To get rid of it they would like to create a new ini on startup but recreating the ini looses values like SECRET_KEY. This app will extract these values from the ini.

Then the current environment-to-ini should be the right program to improve.

It already supports using an existing ini to create a new ini, now, we only need to make it support "only use specialized keys from existing ini":

cp app.ini app-old.ini
environment-to-ini --config app-old.ini --out app.ini --keep "[security].SECRET_KEY" --keep ....

@justusbunsi
Copy link
Member Author

@KN4CK3R Thank you for the summary. 👍

@wxiaoguang Extending the environment-to-ini seems like a better approach. I wasn't aware that it can copy/migrate. I'll try implementing it.
Would I have to pass --keep "[].RUN_MODE" to keep the global RUN_MODE (hypothetically)?

@wxiaoguang
Copy link
Contributor

Would I have to pass --keep "[].RUN_MODE" to keep the global RUN_MODE (hypothetically)?

There is no --keep support at the moment, I mean we could implement it.

IIRC global keys are written as RUN_MODE in the ini system.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Nov 29, 2024

Maybe there could be another approach (I am not sure which one is better)

If we'd like to "revert a key to default", maybe we could pass a special env value to it, eg: GITEA____RUN_MODE=__RESET_DEFAULT__, then environment-to-ini could delete such keys from the ini.


Actually for most cases, empty string should be enough, empty string in ini means default: GITEA____RUN_MODE=

@justusbunsi
Copy link
Member Author

There is no --keep support at the moment, I mean we could implement it.

Yep, I'll try to implement this flag. My question regarding RUN_MODE was to discuss how such global keys could be passed to the new flag.

If we'd like to "revert a key to default", maybe we could pass a special env value to it.

That's not really possible. You cannot know dropped settings because there is no env variable anymore. The --keep approach seems suitable. There are just few settings that are unchangeable for an existing instance. Trying to reset all the others would be huge overhead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/go Pull requests that update Go code modifies/internal size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants