-
Notifications
You must be signed in to change notification settings - Fork 5
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
chore: bump deps #166
Merged
Merged
chore: bump deps #166
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
out of my curiousity, what is the reason to bump so much aggressively? this prevents using easily lower versions if these versions are buggy for whatever reason in Omni.
Does this library work any better with new
grpc
package?I can understand bumping
etcd
, as it's clearly the dependency here and the core thing ofstate-etcd
, but why protobuf? Same question for COSI runtime itself?Why is the fix buried inside
chore: bump 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.
So, it's mostly a hypothesis but it comes from the suggestion that since
omni
imports so many libraries directly or indirectly, some of those breakages can be caught duringlint-test
run in our libraries before we get them into Omni. This also also "splits" required work.I'm coming from this thought:
omni
importsstate-etcd
which imports a lot of third party libraries (Set A
). Some of thisSet A
are also direct imports for Omni (Set B ∩ Set A
) or direct imports of omni dependencies (Set C ∩ Set A
). Going on updates aggressively means (updatingSet A
) means we can always revert those changes/fix those before we are forced to update to those dependencies because our direct imports with fixes that we need updated those imports themselves.Another thought, is that while some of our 3rd party modules tend to stay updated, others do not. That creates complicated situation when, for example updating
Set A
breaksSet B
. This should not happen but in practice in happened before.But this also creates unpleasant complication that in bad case scenario we have to revert our module which is imported by
omni
if Omni breaks on production, and we will have to revert this change. I don't have a good answer for that tho.We broke our code transitively before when our updates to COSI
runtime
brokeomni
. Sincestate-etcd
is a part of call-chain between omni and a COSIruntime
, I think it makes sense to update runtime here beforeomni
.There is no fix in this PR, 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.
There were changes that changed the way the bootstrap list is cleared.
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.
Ah, I removed those. During experiments I assumed that if we
return
before we reachbootstrapList = nil
this will prevent GC from collectingbootstrapList
while we are executingdefer
s which can be arbitrary long. But it doesn't look like the case: GC is smart enough to figure out those are no longer used. Actually I'm not sure that evenbootstrapList = nil
does anything useful at this point.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.
It's a tough question about dependencies, but it's also true that libraries should allow some flexibility for consumers on picking up the version they want, as if libraries pull latest and greatest, there is no flexibility whatsoever. So I guess both points are valid (yours and mine), and I'm not sure which one is the best approach.
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.
Let's merge this one, and probably figure our strategy on updating stuff (Planning? Separate meeting?) and set it in paper. I think we need one anyway, because right now updating stuff relies mostly on intuitive desicion rather than rational one.