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 default host for different database #17873

Closed
wants to merge 1 commit into from

Conversation

lunny
Copy link
Member

@lunny lunny commented Dec 1, 2021

When set DBType as postgres but left HOST empty, then the default value of host in installation page will become 127.0.0.1:3306. It's totally wrong.

@lunny lunny added the type/enhancement An improvement of existing functionality label Dec 1, 2021
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Dec 1, 2021
@wxiaoguang
Copy link
Contributor

I think it can be fixed in https://github.com/go-gitea/gitea/blob/main/web_src/js/features/install.js

Then we can have a unique behavior, instead of splitting code into backend and frontend.

@silverwind
Copy link
Member

I think it can be fixed in https://github.com/go-gitea/gitea/blob/main/web_src/js/features/install.js

But that applies only to installation, what about already installed apps that just have this configuration wrong? I think certain duplication may be necessary.

@wxiaoguang
Copy link
Contributor

For installed config, if the HOST is lost, then the user's config file must have been corrupted, we should not guess a potential incorrect config option for the user either. Doing too many uncertain things will cause more troubles.

@lunny lunny added the pr/wip This PR is not ready for review label Dec 2, 2021
@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 2, 2021
@6543 6543 removed the pr/wip This PR is not ready for review label Dec 2, 2021
@6543
Copy link
Member

6543 commented Dec 2, 2021

I think it should go in as is ... the UI part is another thing - witch should be fixed by this too ...

@6543 6543 added this to the 1.16.0 milestone Dec 2, 2021
@wxiaoguang
Copy link
Contributor

The UI is the key problem, the purpose of this PR is to make the install page show correct default database host, it can be fixed in JS clearly. If we do not want to fix the install UI, we do not need to patch anything.

Changing the default values of the setting module in backend may bring some negative effects:

  1. It splits the default values from install page, and it makes it difficult to keep them the same in future (some developers may forget one of them).
  2. This changed behavior is not documented. No one has the expectation that HOST has default values (and the default values differ by TYPE)
  3. If we want to refactor the setting module in future, such default values may lead to breaking changes.

Since this PR was marked to wip by lunny, I think we can keep it as wip and wait for lunny's thoughts.

@wxiaoguang wxiaoguang added the pr/wip This PR is not ready for review label Dec 3, 2021
Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

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

marking as blocked per @wxiaoguang's feedback

@lunny
Copy link
Member Author

lunny commented Dec 6, 2021

replaced by #17919

@lunny lunny closed this Dec 6, 2021
@lunny lunny deleted the lunny/default_host branch December 6, 2021 09:34
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. pr/wip This PR is not ready for review type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants