Fix go get response if only app URL is custom in configuration by lafriks · Pull Request #2634 · go-gitea/gitea

lafriks · 2017-10-01T14:18:23Z

lunny · 2017-10-01T14:32:53Z

need return?

codecov-io · 2017-10-01T14:36:32Z

Codecov Report

Merging #2634 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2634   +/-   ##
=======================================
  Coverage   27.11%   27.11%           
=======================================
  Files          86       86           
  Lines       17064    17064           
=======================================
  Hits         4627     4627           
  Misses      11759    11759           
  Partials      678      678

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 a8717e5...e946f81. Read the comment docs.

lafriks · 2017-10-01T15:15:16Z

@lunny fixed

ethantkoenig · 2017-10-01T15:31:16Z

LGTM

sapk · 2017-10-02T00:28:28Z

LGTM

For futur work, couldn't we set setting.Domain at appURL.Hostname globally (if domain is not set in ini) ? If it's ok I could do it.

lafriks · 2017-10-02T08:28:24Z

@ethantkoenig @sapk rewritten as requested and setting.Domain should now always match domain in AppURL except when AppURL has IP adress not domain in host part of URL.

sapk · 2017-10-02T09:04:35Z

+	// TODO: Can be replaced with url.Hostname() when minimal GoLang version is 1.8
+	urlHostname := strings.SplitN(url.Host, ":", 2)[0]
+	if urlHostname != Domain && net.ParseIP(urlHostname) == nil {
+		Domain = urlHostname


Domain as it used should contain port at least for the use maid in :

gitea/modules/context/repo.go

Line 138 in 2ef8b8b

func ComposeGoGetImport(owner, repo string) string {

path.Join(setting.Domain, setting.AppSubURL, owner, repo)

It would be better to update the ComposeGoGetImport to add HTTP_PORT

@sapk no it should not contain port as it generates go import part of repository and go imports can not contain port

Sorry mislead HTTP_PORT is not necessary the same as the public port ... but we still need to add the url port to the go-import.

ok sorry that kind of strange how it works if the host is not on defaut port ?

oh sorry I understand that the repo url after package that contain the port.

Yes git url is generated in other place. This is just import part

sapk · 2017-10-02T09:30:35Z

New version LGTM.

…tea#2634) * Fix go get response if only app URL is custom in configuration * Rewrite to update Domain setting to match AppURL

#2640) * Fix go get response if only app URL is custom in configuration * Rewrite to update Domain setting to match AppURL

lafriks added type/bug backport/v1.2 labels Oct 1, 2017

lafriks added this to the 1.3.0 milestone Oct 1, 2017

lunny reviewed Oct 1, 2017

View reviewed changes

Comment thread modules/context/repo.go Outdated

lunny Oct 1, 2017

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

need return?

lafriks reacted with thumbs up emoji

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 1, 2017

Fix go get response if only app URL is custom in configuration

e4dc106

lafriks force-pushed the fix_go_get_url branch from e345d6f to e4dc106 Compare October 1, 2017 15:14

tboerger 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 Oct 1, 2017

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 Oct 2, 2017

Rewrite to update Domain setting to match AppURL

97ee427

lafriks force-pushed the fix_go_get_url branch from 936dc0d to 97ee427 Compare October 2, 2017 08:22

Merge branch 'master' into fix_go_get_url

e946f81

sapk reviewed Oct 2, 2017

View reviewed changes

sapk approved these changes Oct 2, 2017

View reviewed changes

lunny merged commit 46cc45f into go-gitea:master Oct 2, 2017

lafriks added the backport/done All backports for this PR have been created label Oct 2, 2017

lafriks added a commit that referenced this pull request Oct 3, 2017

Fix go get response if only app URL is custom in configuration (#2634) (

5b36379

#2640) * Fix go get response if only app URL is custom in configuration * Rewrite to update Domain setting to match AppURL

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Uh oh!

Uh oh!

Conversation

lafriks commented Oct 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Oct 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lafriks commented Oct 1, 2017

Uh oh!

ethantkoenig commented Oct 1, 2017

Uh oh!

sapk commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lafriks commented Oct 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sapk commented Oct 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Oct 1, 2017 •

edited

Loading

sapk commented Oct 2, 2017 •

edited

Loading