-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Removed un-needed python dependencies #5389
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
Conversation
@@ -22,8 +22,6 @@ jsonfield==2.0.2 | |||
requests==2.21.0 | |||
requests-toolbelt==0.9.1 | |||
slumber==0.7.1 | |||
lxml==4.3.1 | |||
defusedxml==0.5.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 think those are needed when building without using the docker images. We should test that people building without docker can still use their local instance before merging.
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 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.
We shouldn't care about people building without Docker. It's unsupported.
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.
Have we ever officially documented that we don't support this? I think it's useful for development and much easier for users when they are getting started. Making users install and download docker images adds a huge dependency to people trying to get started.
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.
We only have discussed this, but I agree, It would make harder to start contributing.
Anyway, these libs were needed for api v1.
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.
We didn't document this, but we talked in different opportunities. Most (or all?) of the build changes are tested under the process that uses Docker, so we can't guarantee that local building produces the same output than building with Docker.
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.
We still need to keep come libs here
@stsewd Made the change. Please review. |
@Mike-Dai considering that we are removing APIv1, please put the XML requirement together and add a comment above it saying that this has to be removed once v1 is removed (linking the PR that removes it and the PR that adds them as dependencies) |
Thanks! |
Fixes: #5101
Checked
pip.txt
and removed un-needed python dependencies.