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.

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

Merged
merged 9 commits into from
Apr 30, 2020

Conversation

annanay25
Copy link
Contributor

@annanay25 annanay25 commented Apr 28, 2020

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:

github.com/weaveworks/common v0.0.0-20200310113808-2708ba4e60a4 -> github.com/weaveworks/common v0.0.0-20200422083114-5790e7482ff8

Which issue(s) this PR fixes:
Part of #2350

Checklist

  • NA - Tests updated
  • Documentation added
  • 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:
    • clientv3: Upgrade to round robin balancer based on gRPC 1.12 balancer API. We could use this in the clients to resolved DNS addresses.
  • golang.google.org/grpc: No major breaking changes.
  • gopkg.in/yaml.v2: No major changes.

Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
Copy link
Contributor

@pracucci pracucci left a 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>
@annanay25
Copy link
Contributor Author

Thanks for pointing that out @pracucci. The docs pointed out the need for a new DiapOption for the grpc client connecting to etcd: grpc.WithBlock()

I've updated the PR with this new option.

Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

LGTM

@annanay25
Copy link
Contributor Author

I suggest we hold merging this until weaveworks/common#190 is merged

I'll re-update the dependencies :)

Copy link
Contributor

@pracucci pracucci left a 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.

pkg/ring/kv/etcd/etcd.go Outdated Show resolved Hide resolved
@bboreham
Copy link
Contributor

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.
I read through all the diffs, apart from etcd, and the only thing that caught my eye was turning off html escaping in logs.

Signed-off-by: Annanay <annanayagarwal@gmail.com>
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM

@annanay25
Copy link
Contributor Author

I read through all the diffs, apart from etcd, and the only thing that caught my eye was turning off html escaping in logs.

@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?

@bboreham
Copy link
Contributor

I think you misunderstand (or maybe I do). Any time the code was logging &, that would come out as &amp before, and it will be & now. So that is a user-visible change; conceivably someone is parsing logs and they will need to react.

No, it's not something I am concerned about. Just a fact, one that would go in the CHANGELOG.

@annanay25
Copy link
Contributor Author

Thanks for the explanation, I'll update the CHANGELOG per your comments.

Signed-off-by: Annanay <annanayagarwal@gmail.com>
Signed-off-by: Annanay <annanayagarwal@gmail.com>
@gouthamve gouthamve merged commit ef28d34 into cortexproject:master Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants