-
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
Use flagext module from grafana/dskit #4443
Conversation
Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
This removes pkg/util/flagext and starts making use of the code from dskit. This is mostly a mechanical change with a few exceptions * Some extra configuration is now included anywhere we instantiate a weaveworks/common server due to upstream changes (this mostly happens in tests). * Lints for using util_log had to be disabled in some places so that deprecated CLI flags could be registered (which now requires a logger). Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.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.
Thanks for working on this! I left a couple of comments and then LGTM.
Side node: I'm a bit concerned that to just update dskit we've to update a bunch of dependencies. This may introduce slowdowns when reviewing changes. For this time, I did my best to review all dependencies, but we should have a conversation to find a better workflow here.
The following dep upgrade LGTM:
- github.com/hashicorp/consul/api from v1.8.1 to v1.9.1: checked the "api" changes in the CHANGELOG and haven't seen any breaking change (only improvements and fixes)
- github.com/golang/snappy from v0.0.3 to v0.0.4. A couple of changes:
- github.com/gorilla/mux from v1.7.3 to v1.8.0: checked the release notes, should be good to us
- github.com/spf13/afero from v1.2.2 to v1.3.4: checked the release notes and haven't seen any breaking changes
- github.com/gogo/status from v1.0.3 to v1.1.0: checked the release notes and only Go modules support was introduced
- github.com/grpc-ecosystem/go-grpc-middleware from v1.2.2to v1.3.0: checked the diff mentioned in release notes and I don't see any breaking change
- github.com/weaveworks/common from v0.0.0-20210419092856-009d1eebd624 to v0.0.0-20210722103813-e649eff5ab4a: added HTTPListenNetwork and GRPCListenNetwork, which looks correctly handled in this PR
- go.etcd.io/bbolt from v1.3.5 to v1.3.6: according to release notes just a fix
- google.golang.org/api from v0.48.0 to v0.50.0: according to release notes, it just regenerated regenerate discovery clients
- go.etcd.io/etcd/server/v3 from v3.5.0-alpha.0.0.20210225194612-fa82d11a958a to v3.5.0: moving to stale version, LGTM
- github.com/thanos-io/thanos from v0.19.1-0.20210729154440-aa148f8fdb28 to v0.22.0: few minor changes to code paths not even used by Cortex
google.golang.org/api v0.48.0 | ||
google.golang.org/grpc v1.38.0 | ||
google.golang.org/api v0.50.0 | ||
google.golang.org/grpc v1.39.0 |
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 suggest to pin 1.38.0 with this comment:
// TODO review the change introduced by https://github.com/grpc/grpc-go/pull/4416 before upgrading to 1.39.0
replace google.golang.org/grpc => google.golang.org/grpc v1.38.0
Namespace: "cortex", | ||
Name: "deprecated_flags_inuse_total", | ||
Help: "The number of deprecated flags currently set.", | ||
Name: "deprecated_flags_inuse_total", |
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.
This is a breaking change to annotate in the CHANGELOG.
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.
Cortex changes look good.
Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
Thanks for the review! I'll try to find a way to not include so many dependency updates in the future. |
Not your fault! I think we should get better at how we specify dependencies in dskit. Let's continue the conversation offline. |
The options are |
This reverts commit a9a96cb. Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
This reverts commit a9a96cb. Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
This reverts commit a9a96cb. Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
* Update vendor of grafana/dskit Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com> * Use flagext module from grafana/dskit This removes pkg/util/flagext and starts making use of the code from dskit. This is mostly a mechanical change with a few exceptions * Some extra configuration is now included anywhere we instantiate a weaveworks/common server due to upstream changes (this mostly happens in tests). * Lints for using util_log had to be disabled in some places so that deprecated CLI flags could be registered (which now requires a logger). Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com> * Documentation update for weaveworks/common/server Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com> * Code review changes Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com> Signed-off-by: Alvin Lin <alvinlin@amazon.com>
What this PR does:
This removes pkg/util/flagext and starts making use of the code
from dskit. This is mostly a mechanical change with a few exceptions
a weaveworks/common server due to upstream changes (this mostly
happens in tests).
deprecated CLI flags could be registered (which now requires a
logger).
Notes to reviewer:
Vendor updates are in a separate commit to hopefully make review a bit easier.
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]