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.

Sign up for GitHub

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

Jump to bottom

Add UseCompatSSHURI setting #2356

Merged
merged 2 commits into from
Aug 26, 2017
Merged

Add UseCompatSSHURI setting #2356

merged 2 commits into from
Aug 26, 2017

Conversation

ElectronicWar
Copy link
Contributor

@ElectronicWar ElectronicWar commented Aug 22, 2017 •
edited
Loading

This adds the Repository.UseCompatSSHURI boolean setting.
It forces the clone URL box of a repo to always show ssh://user@server/path URL instead of the scp-style URI user@server:path when the default SSH port 22 is used.

I tried to keep it as simple as possible and the changes minimal.
Feedback appreciated.

@lunny lunny added the type/enhancement An improvement of existing functionality label Aug 23, 2017
@lunny lunny added this to the 1.x.x milestone Aug 23, 2017
jonasfranz
jonasfranz suggested changes Aug 23, 2017
View reviewed changes
conf/app.ini Outdated
@@ -23,6 +23,8 @@ PULL_REQUEST_QUEUE_LENGTH = 1000
PREFERRED_LICENSES = Apache License 2.0,MIT License
; Disable ability to interact with repositories by HTTP protocol
DISABLE_HTTP_GIT = false
; Force ssh:// clone url instead of scp-style uri when default SSH port is used
FORCE_CLONE_SSH_URL = false
Copy link
Member

@jonasfranz jonasfranz Aug 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

FORCE_CLONE_SSH_URL might be a little bit misleading because it could also mean that the entered url is the forced clone ssh url. I would use USE_COMPAT_SSH_URI.

Copy link
Member

Choose a reason for hiding this comment

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

Or FORCE_COMPAT_SSH_URI (since it is a compat scheme)

Copy link
Member

Choose a reason for hiding this comment

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

Do not FORCE better USE

Copy link
Member

Choose a reason for hiding this comment

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

@lafriks @bkcsoft Updated comments to USE_COMPAT_SSH_URI.

modules/setting/setting.go Outdated
@@ -161,6 +161,7 @@ var (
PullRequestQueueLength int
PreferredLicenses []string
DisableHTTPGit bool
ForceCloneSSHURL bool
Copy link
Member

@jonasfranz jonasfranz Aug 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This property name is misleading too because it could also be the forced clone ssh url (developers who are not involved in this issue may have problems with this). I would use UseCompatSSHURI.

@lunny lunny modified the milestones: 1.3.0, 1.x.x Aug 23, 2017
@ElectronicWar
Add UseCompatSSHURI setting
6216a90 
Signed-off-by: Manuel Kroeber <manuel.kroeber@gmail.com> (+1 squashed commits)

Squashed commits:

[dda2dc79] Add ForceCloneSSHURL setting

Signed-off-by: Manuel Kroeber <manuel.kroeber@gmail.com>
@ElectronicWar ElectronicWar changed the title Add ForceCloneSSHURL setting Add UseCompatSSHURI setting Aug 23, 2017
bkcsoft
bkcsoft suggested changes Aug 23, 2017
View reviewed changes
Copy link
Member

@bkcsoft bkcsoft left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

models/repo.go
@@ -792,7 +793,11 @@ func (repo *Repository) cloneLink(isWiki bool) *CloneLink {
if setting.SSH.Port != 22 {
Copy link
Member

Choose a reason for hiding this comment

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

Easier 🙂

if setting.Repository.UseCompatSSHURI || setting.SSH.Port != 22 {
  ...
else {
 ...
}

Copy link
Contributor Author

@ElectronicWar ElectronicWar Aug 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Not entirely correct tho, this would produce ssh://user@server:22/path URL when the standard port is used. Would work, but not super pretty in my opinion.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe this one?

if setting.SSH.POrt != 22 {
    ...
} else if setting.Repository.UseCompatSSHURI {
    ...
} else {
    ...
}

Copy link
Member

Choose a reason for hiding this comment

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

True, ☝️ looks nicer though 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, made the change with commit a34402f

@ElectronicWar
Make protocol if construct more readable
a34402f 
Signed-off-by: Manuel Kroeber <manuel.kroeber@gmail.com>
@lafriks
Copy link
Member

lafriks commented Aug 24, 2017

LGTM

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

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 Aug 26, 2017
@tboerger tboerger merged commit d9d8fad into go-gitea:master Aug 26, 2017
@ElectronicWar ElectronicWar deleted the add-sshurl-option branch August 26, 2017 14:15
@lafriks
Copy link
Member

lafriks commented Aug 28, 2017

Please do not merge PR where there are unapproved reviews

@lunny
Copy link
Member

lunny commented Aug 28, 2017

@tboerger yeah. really there are two unapproved reviews.

@tboerger
Copy link
Member

There was a single change request by @bkcsoft and this single change request have been resolved exactly like it have been commented.

@bkcsoft
Copy link
Member

bkcsoft commented Aug 28, 2017

Agree with @tboerger. I had a change request that stated "otherwise LGTM" and had been implemented to the letter, so I'm fine with this merge 🙂

@lafriks
Copy link
Member

lafriks commented Aug 29, 2017

@bkcsoft I'm not against this merge as per principle to not merge while reviewers have not approved their review

@tboerger
Copy link
Member

If there are open requests I'm on your side, but for me this pr was pretty clear.

@lafriks
Copy link
Member

lafriks commented Aug 29, 2017

I know that why also gave my LG-TM but I thought we have rule not to merge with unapproved reviews

@bkcsoft
Copy link
Member

bkcsoft commented Aug 31, 2017

@lafriks I had approved it, since the requested changes had been implemented almost verbatim and I gave my LGTM given that condition.

@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.
Reviewers

@lafriks lafriks lafriks left review comments

@bkcsoft bkcsoft bkcsoft requested changes

@daviian daviian daviian left review comments

@jonasfranz jonasfranz jonasfranz requested changes

@tboerger tboerger tboerger approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.3.0
Development

Successfully merging this pull request may close these issues.
7 participants
@ElectronicWar @lafriks @tboerger @lunny @bkcsoft @daviian @jonasfranz