-
Notifications
You must be signed in to change notification settings - Fork 810
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 weaveworks/common dependency version #2535
Conversation
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
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 went through the changes and everything LGTM unless etcd. It's not that etcd is not good, but the change set is a quite significative given we're upgrading from etcd client 3.3.0 to 3.4.3:
- Version = "3.3.0+git"
+ Version = "3.4.3"
There's an upgrade guide here. Could you go through it (only for what concern the etcd client) and see what impact us, please?
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Thanks for pointing that out @pracucci. The docs pointed out the need for a new I've updated the PR with this new option. |
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.
LGTM
I suggest we hold merging this until weaveworks/common#190 is merged I'll re-update the dependencies :) |
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.
Thanks @annanay25 for your patience on this. I marked as "request for change" just to avoid any other maintainer merge it before we clarify the WithBlock()
issue.
I think your list of "what changed" should highlight the addition of TLS options. Also I like to know there are no subtle changes, as well as no breaking changes. |
Signed-off-by: Annanay <annanayagarwal@gmail.com>
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.
LGTM
@bboreham - I could not find instances of unescaped HTML logging within the repo, so I do not think this should affect us. Is there something specific that you are concerned about? |
I think you misunderstand (or maybe I do). Any time the code was logging No, it's not something I am concerned about. Just a fact, one that would go in the CHANGELOG. |
Thanks for the explanation, I'll update the |
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay annanayagarwal@gmail.com
What this PR does:
This PR is a pre-req for #2502, since it got too big for a review.
The only changes in this PR are a vendor dependency update:
Which issue(s) this PR fixes:
Part of #2350
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]
What changed:
weaveworks/common
: TLS config options now available for the server.aws-sdk-go
: No breaking changes to components used by cortex.go-kit/log
: Turn off HTML escaping in json logger.golang/protobuf
: Interesting change to allow for client-side load-balancing for grpc.gorilla/mux
: Improve CORS Method Middleware. We must now specify an OPTIONS method matcher for the middleware to set CORS headers.go.etcd.io/etcd
:golang.google.org/grpc
: No major breaking changes.gopkg.in/yaml.v2
: No major changes.