make sourcegraph support redis password #3221
Conversation
alexandnpu
added some commits
Apr 4, 2019
beyang
self-assigned this
Apr 4, 2019
beyang
reviewed
Apr 5, 2019
Thanks for submitting this! Have a quick inline comment now, but will do a more thorough review of this shortly. |
} | ||
|
||
var host string | ||
if len(parsedUrl.Opaque) > 0 { |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
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.
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.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
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?
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?
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
alexandnpu
Apr 8, 2019
Author
I've updated my way of parsing redis connection string and added some tests. Please have a review.
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.
alexandnpu
force-pushed the
alexandnpu:feature/make_redis_support_password
branch
from
599667e
to
81008f4
Apr 8, 2019
beyang
added this to the 3.4 milestone
Apr 18, 2019
chrismwendt
referenced this pull request
Apr 18, 2019
Open
Code intel sometimes doesn't work on GitHub PRs #3480
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
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.
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. |
beyang
closed this
Apr 18, 2019
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
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, :).
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: |
alexandnpu commentedApr 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.