Skip to content

Declare DOMAIN variable for docker setup #10780

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

Merged
merged 6 commits into from
May 4, 2020
Merged

Declare DOMAIN variable for docker setup #10780

merged 6 commits into from
May 4, 2020

Conversation

apoiget
Copy link
Contributor

@apoiget apoiget commented Mar 20, 2020

In the /install form, the value for SSH Server Domain is taken form the DOMAIN variable
and overwrites SSH_DOMAIN environment variable set the first time if nothing done

I'm configuring a docker-compose for gitea 1.11.3, I changed SSH_DOMAIN environment, the app.ini takes the variable value but when I go to /install , the value is "localhost" and SSH_DOMAIN is overwritten if i don't change it.
I looked a the code and i saw
cfg.Section("server").Key("SSH_DOMAIN").SetValue(form.Domain)
cfg.Section("server").Key("DOMAIN").SetValue(form.Domain)
https://github.com/go-gitea/gitea/blob/v1.11.3/routers/install.go#L278
so the variable DOMAIN overwrites (which is localhost by default) and I didn't see a way to define it with env variable (https://github.com/go-gitea/gitea/blob/v1.11.3/docker/root/etc/templates/app.ini ).

Comment on lines 256 to 257
* `DOMAIN`: **localhost**: Domain name of this server, used for the displayed clone URL in Gitea's UI.
* `SSH_DOMAIN`: **""**: Domain name of this server, used for the displayed clone URL in Gitea's UI.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* `DOMAIN`: **localhost**: Domain name of this server, used for the displayed clone URL in Gitea's UI.
* `SSH_DOMAIN`: **""**: Domain name of this server, used for the displayed clone URL in Gitea's UI.
* `DOMAIN`: **localhost**: Domain name of this server, used for the displayed http clone URL in Gitea's UI.
* `SSH_DOMAIN`: **""**: Domain name of this server, used for the displayed ssh clone URL in Gitea's UI.

Just to make it clear what the difference is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but during the /install page, the line for the SSH Domain Server takes the DOMAIN variable

Copy link
Member

Choose a reason for hiding this comment

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

I aggree with @gary-kim

Copy link
Member

@gary-kim gary-kim Apr 15, 2020

Choose a reason for hiding this comment

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

but during the /install page, the line for the SSH Domain Server takes the DOMAIN variable

You don't have to take my suggested change as is, feel free to change it to what you feel makes the difference and purpose clear. Just want to make sure no one asks why there are two different environment variables for the same setting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, thanks for the feedback. I have done some modifications.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 6, 2020
In the /install form, the value for SSH Server Domain is taken form the DOMAIN variable
and overwrites SSH_DOMAIN environment variable set the first time if nothing done
@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 Apr 20, 2020
@lafriks
Copy link
Member

lafriks commented Apr 27, 2020

@gary-kim please review

@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 Apr 28, 2020
@codecov-io
Copy link

Codecov Report

Merging #10780 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10780      +/-   ##
==========================================
- Coverage   43.28%   43.27%   -0.02%     
==========================================
  Files         605      605              
  Lines       86207    86207              
==========================================
- Hits        37319    37304      -15     
- Misses      44292    44304      +12     
- Partials     4596     4599       +3     
Impacted Files Coverage Δ
services/pull/check.go 52.43% <0.00%> (-4.27%) ⬇️
modules/git/command.go 86.95% <0.00%> (-2.61%) ⬇️
modules/queue/unique_queue_disk_channel.go 53.84% <0.00%> (-1.93%) ⬇️
models/repo_list.go 76.51% <0.00%> (-0.81%) ⬇️
services/pull/pull.go 33.39% <0.00%> (-0.58%) ⬇️
modules/git/repo.go 50.62% <0.00%> (ø)
modules/indexer/stats/db.go 59.37% <0.00%> (+9.37%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b6f20b...48f85c1. Read the comment docs.

@zeripath zeripath merged commit 99082ee into go-gitea:master May 4, 2020
@zeripath zeripath changed the title Fix; declare DOMAIN variable for docker setup Declare DOMAIN variable for docker setup May 4, 2020
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
In the /install form, the value for SSH Server Domain is taken form the DOMAIN variable
and overwrites SSH_DOMAIN environment variable set the first time if nothing done

Co-authored-by: Adrian POIGET <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
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. topic/deployment type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants