-
Notifications
You must be signed in to change notification settings - Fork 241
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
Bump yelp-clog and scribereader #3868
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.
lgtm!
@yaroliakh did you also verify that |
@@ -32,7 +32,7 @@ srv-configs==1.1.0 | |||
sticht[yelp_internal]==1.1.18 | |||
vault-tools==0.9.2 | |||
yelp-cgeom==1.3.1 | |||
yelp-clog==5.2.3 | |||
yelp-clog==7.1.1 |
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 think we'll want to add tenacity
as well since that's now required by yelp-clog
(figuring this out was kinda annoying: i added the direct dependencies (i.e., things we import) into requirements-minimal.txt and copied the contents here to requirements.txt so that i could run check-requirements
)
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.
there's also some lines we can delete: https://fluffy.yelpcorp.com/i/wmSXXqXBvW1THmZPs5mfNGfF2cR9l69g.html is what i think we'd want as a final state
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.
So I've pinned deps and ran check-requirements
for the following main dependencies:
yelp-clog
scribereader
clusterman-metrics
vault-tools
slo-transcoder
yelp-profiling
sticht[yelp_internal]==1.1.18
Not sure about deleting other deps, as I can see most of them being referenced in code
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 good catch - i somehow missed grepping for clusterman-metrics (which is maybe why my check-requirements said we could delete some deps)
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.
also: big thank you for annotating these dependencies :)
seems to be working, as |
@@ -32,7 +32,7 @@ srv-configs==1.1.0 | |||
sticht[yelp_internal]==1.1.18 | |||
vault-tools==0.9.2 | |||
yelp-cgeom==1.3.1 | |||
yelp-clog==5.2.3 | |||
yelp-clog==7.1.1 |
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 good catch - i somehow missed grepping for clusterman-metrics (which is maybe why my check-requirements said we could delete some deps)
@@ -32,7 +32,7 @@ srv-configs==1.1.0 | |||
sticht[yelp_internal]==1.1.18 | |||
vault-tools==0.9.2 | |||
yelp-cgeom==1.3.1 | |||
yelp-clog==5.2.3 | |||
yelp-clog==7.1.1 |
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.
also: big thank you for annotating these dependencies :)
Based on the 7+ version changelog:
none of the above seems to be used.
Removing the only reference I could find to deprecated
clog.readers
. Everything else should be already using Monk backend and should work without changes.Smoke test: