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

Do not overwrite the SSH_DOMAIN when installing #25580

Closed
wants to merge 1 commit into from

Conversation

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 29, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 29, 2023
@wxiaoguang wxiaoguang added backport/v1.20 This PR should be backported to Gitea 1.20 type/bug labels Jun 29, 2023
@wxiaoguang wxiaoguang added this to the 1.21.0 milestone Jun 29, 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 Jun 29, 2023
@@ -390,7 +390,7 @@ func SubmitInstall(ctx *context.Context) {
cfg.Section("database").Key("LOG_SQL").SetValue("false") // LOG_SQL is rarely helpful

cfg.Section("repository").Key("ROOT").SetValue(form.RepoRootPath)
cfg.Section("server").Key("SSH_DOMAIN").SetValue(form.Domain)
cfg.Section("server").Key("SSH_DOMAIN").MustString(form.Domain)
Copy link
Member

Choose a reason for hiding this comment

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

Are we really going to convert each SetValue to MustString in a separate PR now?

Copy link
Contributor Author

@wxiaoguang wxiaoguang Jun 29, 2023

Choose a reason for hiding this comment

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

Do you have a better idea for this PR?


Put into draft, let's think more about it ....

Copy link
Member

Choose a reason for hiding this comment

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

That wasn't meant as disapproval, I've simply noticed a tendency lately to replace SetValue in this method with MustString.
I'm wondering how many other settings we will convert as well in subsequent PRs.

@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 Jun 29, 2023
@wxiaoguang wxiaoguang marked this pull request as draft June 29, 2023 08:20
@wxiaoguang
Copy link
Contributor Author

I have a clear idea about this problem now.

Make "install page" respect environment config #25648

@wxiaoguang wxiaoguang closed this Jul 3, 2023
@GiteaBot GiteaBot removed this from the 1.21.0 milestone Jul 3, 2023
@wxiaoguang wxiaoguang deleted the fix-install-overwrite branch July 3, 2023 12:30
silverwind pushed a commit that referenced this pull request Jul 9, 2023
Replace #25580

Fix #19453

The problem was: when users set "GITEA__XXX__YYY" , the "install page"
doesn't respect it.

So, to make the result consistent and avoid surprising end users, now
the "install page" also writes the environment variables to the config
file.

And, to make things clear, there are enough messages on the UI to tell
users what will happen.

There are some necessary/related changes to `environment-to-ini.go`:

* The "--clear" flag is removed and it was incorrectly written there.
The "clear" operation should be done if INSTALL_LOCK=true
* The "--prefix" flag is removed because it's never used, never
documented and it only causes inconsistent behavior.


![image](https://github.com/go-gitea/gitea/assets/2114189/12778ee4-3fb5-4664-a73a-41ebbd77cd5b)
wxiaoguang added a commit to wxiaoguang/gitea that referenced this pull request Jul 10, 2023
Replace go-gitea#25580

Fix go-gitea#19453

The problem was: when users set "GITEA__XXX__YYY" , the "install page"
doesn't respect it.

So, to make the result consistent and avoid surprising end users, now
the "install page" also writes the environment variables to the config
file.

And, to make things clear, there are enough messages on the UI to tell
users what will happen.

There are some necessary/related changes to `environment-to-ini.go`:

* The "--clear" flag is removed and it was incorrectly written there.
The "clear" operation should be done if INSTALL_LOCK=true
* The "--prefix" flag is removed because it's never used, never
documented and it only causes inconsistent behavior.

![image](https://github.com/go-gitea/gitea/assets/2114189/12778ee4-3fb5-4664-a73a-41ebbd77cd5b)
# Conflicts:
#	templates/install.tmpl
KN4CK3R pushed a commit that referenced this pull request Jul 10, 2023
Backport #25648

Replace #25580

Fix #19453

The problem was: when users set "GITEA__XXX__YYY" , the "install page"
doesn't respect it.

So, to make the result consistent and avoid surprising end users, now
the "install page" also writes the environment variables to the config
file.

And, to make things clear, there are enough messages on the UI to tell
users what will happen.

There are some necessary/related changes to `environment-to-ini.go`:

* The "--clear" flag is removed and it was incorrectly written there.
The "clear" operation should be done if INSTALL_LOCK=true
* The "--prefix" flag is removed because it's never used, never
documented and it only causes inconsistent behavior.

The only conflict during backport is "ui divider" in
templates/install.tmpl
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Oct 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/v1.20 This PR should be backported to Gitea 1.20 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clone instructions do not respect SSH_DOMAIN variable when set as an environment variable
4 participants