-
Notifications
You must be signed in to change notification settings - Fork 961
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
refactor: undo the great unpinning #1267
Conversation
@@ -29,9 +22,28 @@ | |||
maintainer='Amundsen TSC', | |||
maintainer_email='[email protected]', | |||
packages=find_packages(exclude=['tests*']), | |||
install_requires=requirements_common, | |||
install_requires=[ | |||
# Packages in here should rarely be pinned. This is because these |
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.
note, I didn't author this comment, I took it from the earlier version of amundsen-common
Signed-off-by: Dmitriy Kunitskiy <[email protected]>
Signed-off-by: Dmitriy Kunitskiy <[email protected]>
Signed-off-by: Dmitriy Kunitskiy <[email protected]>
c36d6bd
to
12d9710
Compare
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.
My understanding is there's enough consensus on this route to try it out. we can always go back if not, but I think this is the safest way out of the situation.
* undo the great unpinning Signed-off-by: Dmitriy Kunitskiy <[email protected]> * remove all extra requires Signed-off-by: Dmitriy Kunitskiy <[email protected]> * dev back to all Signed-off-by: Dmitriy Kunitskiy <[email protected]> Signed-off-by: Elena Boal <[email protected]>
* undo the great unpinning Signed-off-by: Dmitriy Kunitskiy <[email protected]> * remove all extra requires Signed-off-by: Dmitriy Kunitskiy <[email protected]> * dev back to all Signed-off-by: Dmitriy Kunitskiy <[email protected]> Signed-off-by: Amom <[email protected]>
* undo the great unpinning Signed-off-by: Dmitriy Kunitskiy <[email protected]> * remove all extra requires Signed-off-by: Dmitriy Kunitskiy <[email protected]> * dev back to all Signed-off-by: Dmitriy Kunitskiy <[email protected]>
* undo the great unpinning Signed-off-by: Dmitriy Kunitskiy <[email protected]> * remove all extra requires Signed-off-by: Dmitriy Kunitskiy <[email protected]> * dev back to all Signed-off-by: Dmitriy Kunitskiy <[email protected]>
Signed-off-by: Dmitriy Kunitskiy [email protected]
Summary of Changes
This formally ends The Great Unpinning. In effect, I realized it does not solve the problem of
amundsen-common
as it still results in this simple library requiring many unrelated dependencies. Additionally, the loss of reproducible builds introduced by unpinned service dependencies is possibly not worth the benefit.Instead, I propose to keep amundsen-common separate with its small requirements set. Why is this okay? Because, when
amundsen-common
is used inside databuilder/frontend/metadata/search, theinstall_requires
ofamundsen-common
will be completely irrelevant. In other words, the pain of desynchronized dependencies will not return and the benefits of @mgorsk1's efforts in #1163 are maintained.One element of the earlier PR I have kept is the removal of outdated dependencies:
Tests
Documentation
CheckList
Make sure you have checked all steps below to ensure a timely review.