Skip to content
  • Unwatch
    Notifications
  • Fork

    Fork sourcegraph

    If this dialog fails to load, you can visit the fork page directly.

/sourcegraph

make sourcegraph support redis password #3221

Conversation

Projects
Projects
None yet
2 participants
Lock conversation

Lock conversation on this pull request

  • Other users can’t add new comments to this pull request.
  • You and other members of teams with write access to this repository can still leave comments that others can see.
  • You can always unlock this pull request again in the future.

Optionally, choose a reason for locking that others can see. Learn more about when it’s appropriate to lock conversations.

@alexandnpu

Pick your reaction

Copy link Report abuse

alexandnpu commented Apr 4, 2019

Test plan:

sourcegraph should be able to support redis with authorization.

Note:
I failed to setup my dev environment to test on my own mac, as the dev setup is not that easy enough for only the server side.
Maybe this PR is an idea to show you that sourcegraph should be able to use password to connect redis. Because in my company, every redis server should use password to connect.

Select a reply ctrl .

The content you are editing has changed. Please try again.

Attach files by dragging & dropping, selecting or pasting them. Uploading your files… We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Attaching documents requires write permission to this repository. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Yowza, that’s a big file with a file smaller than 10MB. This file is empty. with a file that’s not empty. This file is hidden. with another file. Something went really wrong, and we can’t process that file.

Nothing to preview

@beyang beyang self-assigned this Apr 4, 2019

@beyang

Pick your reaction

Copy link Report abuse
Member

beyang left a comment

Thanks for submitting this! Have a quick inline comment now, but will do a more thorough review of this shortly.

Select a reply ctrl .

The content you are editing has changed. Please try again.

Attach files by dragging & dropping, selecting or pasting them. Uploading your files… We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Attaching documents requires write permission to this repository. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Yowza, that’s a big file with a file smaller than 10MB. This file is empty. with a file that’s not empty. This file is hidden. with another file. Something went really wrong, and we can’t process that file.

Nothing to preview

}

var host string
if len(parsedUrl.Opaque) > 0 {

This comment has been minimized.

Copy link
Report abuse
@beyang

beyang Apr 5, 2019

Member

Can you provide an example of a URL with an opaque component that would be used here? Haven't seen one used for connecting to Redis before, and adding that example to a unit test would be beneficial.

Select a reply ctrl .

The content you are editing has changed. Please try again.

Attach files by dragging & dropping, selecting or pasting them. Uploading your files… We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Attaching documents requires write permission to this repository. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Yowza, that’s a big file with a file smaller than 10MB. This file is empty. with a file that’s not empty. This file is hidden. with another file. Something went really wrong, and we can’t process that file.

Nothing to preview

Pick your reaction

Copy link
Reference in new issue

Reference in new issue

sourcegraph
Repositories
Report
@beyang

beyang Apr 5, 2019

Member

Choose a reason for hiding this comment

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

Can you provide an example of a URL with an opaque component that would be used here? Haven't seen one used for connecting to Redis before, and adding that example to a unit test would be beneficial.

Select a reply ctrl .

The content you are editing has changed. Please try again.

Attach files by dragging & dropping, selecting or pasting them. Uploading your files… We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Attaching documents requires write permission to this repository. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Yowza, that’s a big file with a file smaller than 10MB. This file is empty. with a file that’s not empty. This file is hidden. with another file. Something went really wrong, and we can’t process that file.

Nothing to preview

This comment has been minimized.

Copy link
Report abuse
@alexandnpu

alexandnpu Apr 6, 2019

Author

Maybe Opaque is not a very straight forward word to understand. That part of code is handling urls whose pattern is host:port, such as "localhost:5000" or so.

I do not like the way Opaque used here, but it is in the standard library net/url/url.go

// A URL represents a parsed URL (technically, a URI reference).
//
// The general form represented is:
//
//	[scheme:][//[userinfo@]host][/]path[?query][#fragment]
//
// URLs that do not start with a slash after the scheme are interpreted as:
//
//	scheme:opaque[?query][#fragment]
//
// Note that the Path field is stored in decoded form: /%47%6f%2f becomes /Go/.
// A consequence is that it is impossible to tell which slashes in the Path were
// slashes in the raw URL and which were %2f. This distinction is rarely important,
// but when it is, code must not use Path directly.
// The Parse function sets both Path and RawPath in the URL it returns,
// and URL's String method uses RawPath if it is a valid encoding of Path,
// by calling the EscapedPath method.
type URL struct {
	Scheme     string
	Opaque     string    // encoded opaque data
	User       *Userinfo // username and password information
	Host       string    // host or host:port
	Path       string    // path (relative paths may omit leading slash)
	RawPath    string    // encoded path hint (see EscapedPath method)
	ForceQuery bool      // append a query ('?') even if RawQuery is empty
	RawQuery   string    // encoded query values, without '?'
	Fragment   string    // fragment for references, without '#'
}

any suggestions for making this part more elegant?

Select a reply ctrl .

The content you are editing has changed. Please try again.

Attach files by dragging & dropping, selecting or pasting them. Uploading your files… We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Attaching documents requires write permission to this repository. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Yowza, that’s a big file with a file smaller than 10MB. This file is empty. with a file that’s not empty. This file is hidden. with another file. Something went really wrong, and we can’t process that file.

Nothing to preview

Pick your reaction

Copy link
Reference in new issue

Reference in new issue

sourcegraph
Repositories
Report
@alexandnpu

alexandnpu Apr 6, 2019

Author

Choose a reason for hiding this comment

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

Maybe Opaque is not a very straight forward word to understand. That part of code is handling urls whose pattern is host:port, such as "localhost:5000" or so.

I do not like the way Opaque used here, but it is in the standard library net/url/url.go

// A URL represents a parsed URL (technically, a URI reference).
//
// The general form represented is:
//
//	[scheme:][//[userinfo@]host][/]path[?query][#fragment]
//
// URLs that do not start with a slash after the scheme are interpreted as:
//
//	scheme:opaque[?query][#fragment]
//
// Note that the Path field is stored in decoded form: /%47%6f%2f becomes /Go/.
// A consequence is that it is impossible to tell which slashes in the Path were
// slashes in the raw URL and which were %2f. This distinction is rarely important,
// but when it is, code must not use Path directly.
// The Parse function sets both Path and RawPath in the URL it returns,
// and URL's String method uses RawPath if it is a valid encoding of Path,
// by calling the EscapedPath method.
type URL struct {
	Scheme     string
	Opaque     string    // encoded opaque data
	User       *Userinfo // username and password information
	Host       string    // host or host:port
	Path       string    // path (relative paths may omit leading slash)
	RawPath    string    // encoded path hint (see EscapedPath method)
	ForceQuery bool      // append a query ('?') even if RawQuery is empty
	RawQuery   string    // encoded query values, without '?'
	Fragment   string    // fragment for references, without '#'
}

any suggestions for making this part more elegant?

Select a reply ctrl .

The content you are editing has changed. Please try again.

Attach files by dragging & dropping, selecting or pasting them. Uploading your files… We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Attaching documents requires write permission to this repository. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Yowza, that’s a big file with a file smaller than 10MB. This file is empty. with a file that’s not empty. This file is hidden. with another file. Something went really wrong, and we can’t process that file.

Nothing to preview

This comment has been minimized.

Copy link
Report abuse
@alexandnpu

alexandnpu Apr 8, 2019

Author

I've updated my way of parsing redis connection string and added some tests. Please have a review.

Select a reply ctrl .

The content you are editing has changed. Please try again.

Attach files by dragging & dropping, selecting or pasting them. Uploading your files… We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Attaching documents requires write permission to this repository. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Yowza, that’s a big file with a file smaller than 10MB. This file is empty. with a file that’s not empty. This file is hidden. with another file. Something went really wrong, and we can’t process that file.

Nothing to preview

Pick your reaction

Copy link
Reference in new issue

Reference in new issue

sourcegraph
Repositories
Report
@alexandnpu

alexandnpu Apr 8, 2019

Author

Choose a reason for hiding this comment

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

I've updated my way of parsing redis connection string and added some tests. Please have a review.

Select a reply ctrl .

The content you are editing has changed. Please try again.

Attach files by dragging & dropping, selecting or pasting them. Uploading your files… We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Attaching documents requires write permission to this repository. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Yowza, that’s a big file with a file smaller than 10MB. This file is empty. with a file that’s not empty. This file is hidden. with another file. Something went really wrong, and we can’t process that file.

Nothing to preview

@lguychard
Select a reply ctrl .

Attach files by dragging & dropping, selecting or pasting them. Uploading your files… We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Attaching documents requires write permission to this repository. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Yowza, that’s a big file with a file smaller than 10MB. This file is empty. with a file that’s not empty. This file is hidden. with another file. Something went really wrong, and we can’t process that file.

Nothing to preview

@alexandnpu alexandnpu force-pushed the alexandnpu:feature/make_redis_support_password branch from 599667e to 81008f4 Apr 8, 2019

@beyang beyang added this to the 3.4 milestone Apr 18, 2019

@beyang

This comment has been minimized.

Show comment
Hide comment
Copy link
Report abuse
@beyang

beyang Apr 18, 2019

Member

Thanks for pushing up this PR @alexandnpu. I've incorporated this into #3482, so will close this in favor of that. I expect this to ship in 3.4.

Select a reply ctrl .

The content you are editing has changed. Please try again.

Attach files by dragging & dropping, selecting or pasting them. Uploading your files… We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Attaching documents requires write permission to this repository. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Yowza, that’s a big file with a file smaller than 10MB. This file is empty. with a file that’s not empty. This file is hidden. with another file. Something went really wrong, and we can’t process that file.

Nothing to preview

Pick your reaction

Copy link
Reference in new issue

Reference in new issue

sourcegraph
Repositories
Report abuse
Member

beyang commented Apr 18, 2019

Choose a reason for hiding this comment

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

Thanks for pushing up this PR @alexandnpu. I've incorporated this into #3482, so will close this in favor of that. I expect this to ship in 3.4.

Select a reply ctrl .

The content you are editing has changed. Please try again.

Attach files by dragging & dropping, selecting or pasting them. Uploading your files… We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Attaching documents requires write permission to this repository. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Yowza, that’s a big file with a file smaller than 10MB. This file is empty. with a file that’s not empty. This file is hidden. with another file. Something went really wrong, and we can’t process that file.

Nothing to preview

@beyang beyang closed this Apr 18, 2019

@alexandnpu

This comment has been minimized.

Show comment
Hide comment
Copy link
Report abuse
@alexandnpu

alexandnpu Apr 18, 2019

Author

I am really happy that you can accept my idea.

I am new to GO, and your solution is really better than mine. I did not realize that there is DialUrl interface in redigo. And the standard, https://www.iana.org/assignments/uri-schemes/prov/redis.

I learned from you. Thank you!

PS:
I am really expecting it to be released as soon as possible. So my team can use SG again, :).

Select a reply ctrl .

The content you are editing has changed. Please try again.

Attach files by dragging & dropping, selecting or pasting them. Uploading your files… We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Attaching documents requires write permission to this repository. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Yowza, that’s a big file with a file smaller than 10MB. This file is empty. with a file that’s not empty. This file is hidden. with another file. Something went really wrong, and we can’t process that file.

Nothing to preview

Pick your reaction

Copy link
Reference in new issue

Reference in new issue

sourcegraph
Repositories
Report abuse
Author

alexandnpu commented Apr 18, 2019

Choose a reason for hiding this comment

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

I am really happy that you can accept my idea.

I am new to GO, and your solution is really better than mine. I did not realize that there is DialUrl interface in redigo. And the standard, https://www.iana.org/assignments/uri-schemes/prov/redis.

I learned from you. Thank you!

PS:
I am really expecting it to be released as soon as possible. So my team can use SG again, :).

Select a reply ctrl .

The content you are editing has changed. Please try again.

Attach files by dragging & dropping, selecting or pasting them. Uploading your files… We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Attaching documents requires write permission to this repository. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Yowza, that’s a big file with a file smaller than 10MB. This file is empty. with a file that’s not empty. This file is hidden. with another file. Something went really wrong, and we can’t process that file.

Nothing to preview

Couldn’t update branch

Oops, something went wrong.

@lguychard
Select a reply ctrl .

Attach files by dragging & dropping, selecting or pasting them. Uploading your files… We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Attaching documents requires write permission to this repository. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, LOG, PDF, PPTX, TXT, XLSX or ZIP. Yowza, that’s a big file with a file smaller than 10MB. This file is empty. with a file that’s not empty. This file is hidden. with another file. Something went really wrong, and we can’t process that file.

Nothing to preview

ProTip! Add .patch or .diff to the end of URLs for Git’s plaintext views.
You can’t perform that action at this time.