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

Drop unused dependencies #5228

Merged
merged 3 commits into from
Apr 10, 2021
Merged

Drop unused dependencies #5228

merged 3 commits into from
Apr 10, 2021

Conversation

cognifloyd
Copy link
Member

@cognifloyd cognifloyd commented Apr 9, 2021

prometheus_client was part of a metrics driver that was dropped. #4310 (82772cc) #4504 (873c57f) unused since v2.9.0

zipp and more-itertools were py2-only transitive deps. #4843 #4847 (afaict these were transitive deps via wheel) added in v3.2.0; unused since v3.4.0 when we dropped python 2

python-gnupg was dropped with the removal of st2debug. #5103 unused since v3.4.0

background: I discovered this as part of an experiment using pants in st2sandbox/st2@pants to replace the Makefile and related scripts.

@pull-request-size pull-request-size bot added the size/XS PR that changes 0-9 lines. Quick fix/merge. label Apr 9, 2021
@cognifloyd cognifloyd added this to the 3.5.0 milestone Apr 9, 2021
@pull-request-size pull-request-size bot added size/S PR that changes 10-29 lines. Very easy to review. and removed size/XS PR that changes 0-9 lines. Quick fix/merge. labels Apr 9, 2021
prometheus_client was part of a metrics driver that was dropped.
python-gnupg was dropped with the removal of st2debug.
zipp and more-itertools were py2-only transitive deps.
Copy link
Contributor

@amanda11 amanda11 left a comment

Choose a reason for hiding this comment

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

LGTM - but perhaps add a CHANGELOG entry

@cognifloyd
Copy link
Member Author

This is cleanup missed from previous PRs that removed the dependencies. I don't think it warrants a changelog entry.

@arm4b
Copy link
Member

arm4b commented Apr 9, 2021

@cognifloyd For context, which PR # and changelog entry you're referring to?

@blag
Copy link
Contributor

blag commented Apr 9, 2021

@armab That's a bit of a fishing expedition to ask somebody to go on, don't you think? And I don't understand how the results would matter.

I agree with @cognifloyd, I don't know that this is really a big enough change on its own to warrant a line in the changelog. This isn't going to change any user-facing code.

@arm4b
Copy link
Member

arm4b commented Apr 9, 2021

I believe having a Changelog record helps a lot for both users and developers and agree that having it would be nice for consistency with the engineering flow.
Looking deeper, I can't find the 3.5dev record indicating the requirements cleanup and wondering if it's 3.4dev or any other change?
In the past, we included Changelog for the PRs like CI, Tests and other non-user facing items and don't see a problem with updating it here.

@amanda11
Copy link
Contributor

amanda11 commented Apr 9, 2021

To make it simple the changelog could just say remove unused dependencies - rather than needing a full investigation to work out exactly when those dependencies were removed. It states the point of the change, but doens't require lots of effort for what is a nice clean up - which is always appreciated.
But my approval isn't dependant on a change log entry!

@cognifloyd
Copy link
Member Author

I think that's too much noise in the changelog for something like this.

I did all that research when I validated that these were not needed. Let me do it again to provide info. I'll update the original post with details as I find it.

@cognifloyd
Copy link
Member Author

prometheus_client unused since v2.9.0
The other 3 are not needed as of v3.4.0 when we dropped python2 and removed st2debug.

@cognifloyd
Copy link
Member Author

I don't think a changelog entry adds any value, so I'm not sure what to write.
All of you are maintainers, please feel free to add a changelog entry with whatever verbiage you think makes sense.

@arm4b
Copy link
Member

arm4b commented Apr 9, 2021

Something simple like this would be totally fine:

* Cleanup unused ``pip`` dependencies (improvement). #5228

  Contributed by @cognifloyd.

Copy link
Member

@Kami Kami left a comment

Choose a reason for hiding this comment

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

Yay, the more stuff we can remove, the better 👍

But yeah, please still add a changelog entry for just in case.

@Kami
Copy link
Member

Kami commented Apr 9, 2021

Eventually we could also still removing all the Python 2 / 3 compatibility boiler plate and also get rid of six.

@Kami
Copy link
Member

Kami commented Apr 9, 2021

And re changelog entry - IIRC, anything from StackStorm virtualenv is also available to pack virtualenv (but I could also be wrong on that since that behavior had some edge cases and has changed in the past) so in theory a pack could be depending on some of those dependencies without specifying them in requirements.txt, but that's also very unlikely.

So if that's indeed still true / accurate, then it would make sense to explicitly list those dependencies in the changelog entry.

@Kami Kami merged commit 5a08b5b into StackStorm:master Apr 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pantsbuild size/S PR that changes 10-29 lines. Very easy to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants