-
Notifications
You must be signed in to change notification settings - Fork 326
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
Update to latest serviceresolver code #335
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just that small change with the test that checks both ToConsul
and MatchesConsul
but looks great!!!
@@ -7,7 +7,7 @@ require ( | |||
github.com/digitalocean/godo v1.10.0 // indirect | |||
github.com/go-logr/logr v0.1.0 | |||
github.com/google/go-querystring v1.0.0 // indirect | |||
github.com/hashicorp/consul/api v1.6.0 | |||
github.com/hashicorp/consul/api v1.4.1-0.20200924193849-85d223b73c1d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh go-lang versioning. A reasonable person would expect this to be v1.6.1-foobar
, but not golang
8da3144
to
77a8f38
Compare
77a8f38
to
95306dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
} | ||
|
||
type CookieConfig struct { | ||
// Session generates a session cookie with no expiration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be something like this since it's a bool
// Session generates a session cookie with no expiration. | |
// Session determines whether to generate a session cookie with no expiration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's better, will update. I copied these directly from Consul.
} | ||
|
||
// Test MatchesConsul for cases that should return false. | ||
func TestServiceResolver_MatchesConsulFalse(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, why are you removing this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed it because it was testing reflect.DeepEqual
. Although the "different type" case is still valid to test I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ashwin-venkatesh should we add these back in #340?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can do
Changes proposed in this PR:
master
api version which contains new serviceresolver codematchesConsul
implementation to usereflect.DeepEqual
and removes tests to match how we're testing the other methodsCompanion to hashicorp/consul-helm#619