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

Fix installation page ssh domain unavilable #506

Merged
merged 1 commit into from
Jan 4, 2017

Conversation

lunny
Copy link
Member

@lunny lunny commented Dec 27, 2016

Backport from #499

@lunny lunny added the type/bug label Dec 27, 2016
@lunny lunny added this to the 1.0.1 milestone Dec 27, 2016
@Bwko
Copy link
Member

Bwko commented Dec 27, 2016

LGTM

@tboerger tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Dec 27, 2016
@tboerger
Copy link
Member

No, DOMAIN is correct, SSH_DOMAIN should be %(DOMAIN)s. If I see it correctly we won't set the DOMAIN anymore with that change.

@tboerger
Copy link
Member

Beside that this is not a bug and should not be backported to 1.0.1.

@lunny
Copy link
Member Author

lunny commented Dec 29, 2016

Installation page show this input is ssh domain, but it will update domain not ssh domain. Isn't it a bug?

@strk
Copy link
Member

strk commented Dec 29, 2016

There's no mention of SSH_DOMAIN here: https://gogs.io/docs/advanced/configuration_cheat_sheet
What is DOMAIN for, and what SSH_DOMAIN ? Why are both needed ?

@lunny
Copy link
Member Author

lunny commented Dec 31, 2016

Don't know why. Maybe someone use different domain for HTTP and SSH. But this PR is really work.

@strk
Copy link
Member

strk commented Jan 2, 2017

But the form only asks for DOMAIN, not SSH_DOMAIN ?
In that case we should set both variables to the value provided via the form, no ?
Or SSH_DOMAIN should default to the value of DOMAIN if empty...

@bkcsoft
Copy link
Member

bkcsoft commented Jan 3, 2017

From app.ini

; Domain name to be exposed in clone URL
SSH_DOMAIN = %(DOMAIN)s

This PR is invalid 😕


The reason for SSH_DOMAIN vs DOMAIN is that you might have the webui running behind a loadbalancer/proxy on company.com/git that points to git.company.com, but ssh can't be used in such a manner so then you'd need to point to git.company.com

@lunny
Copy link
Member Author

lunny commented Jan 3, 2017

untitled
Please see the installation page, there are Domain:This affects SSH clone URLs. and Application URL:This affects HTTP/HTTPS clone URL and somewhere in email.. So when domain inputed value, which one should we change?

@strk
Copy link
Member

strk commented Jan 3, 2017

Sounds like the Domain in the installation page really refers to SSH Domain then.
But then again what's the difference between ROOT_URL and DOMAIN in the settings ?
Who uses DOMAIN and how ? Is it about email domain ? Should it be also set at installation time ?

@bkcsoft
Copy link
Member

bkcsoft commented Jan 3, 2017

Feels like someone has to go through the install-page and see what needs to be improved, it feels slightly confusing as it is now

@tboerger
Copy link
Member

tboerger commented Jan 4, 2017

LGTM

@tboerger tboerger 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 Jan 4, 2017
@tboerger tboerger merged commit a5ea9b4 into go-gitea:release/v1.0 Jan 4, 2017
@tboerger tboerger added the backport/done All backports for this PR have been created label Jan 5, 2017
@lunny lunny deleted the lunny/fix_install_domain_1.0 branch April 19, 2017 05:45
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants