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

Stop coercing Pipfile source URL's to have trailing slashes #7783

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

jeffwidman
Copy link
Member

I had to stare at this code golf for a little bit to realize that it's enforcing that every source URL has one-and-only-one trailing slash.

I started to tweak it a little to make it more readable, but I decided instead that this is business we really shouldn't be in... Dependabot is the tool that runs the package manager, not the package manager itself.

So if a package manager doesn't like a URL that lacks a trailing slash, it should be the one handling the coercing or raising the error, and then we transparently forward that to the user.

There are some use cases where we need to handle URL normalization for deduping purposes within Dependabot internals, but I poked around a bit and as far as I can tell they don't apply here.

So let's trust the user / package managers to do the right thing (which may mean giving us a clear error).

@jeffwidman jeffwidman requested a review from a team as a code owner August 9, 2023 23:48
@jeffwidman jeffwidman force-pushed the stop-coercing-pipfile-source-urls branch from d69e04d to edd027f Compare August 9, 2023 23:50
@jeffwidman jeffwidman changed the title Stop coercing Pipfile source url's to have trailing slashes Stop coercing Pipfile source URL's to have trailing slashes Aug 9, 2023
@jeffwidman
Copy link
Member Author

@matteius @oz123 can I get your take on this from the perspective of pipenv maintainers?

@matteius
Copy link

@jeffwidman That makes sense to me and my take is that if the source URL were in the Pipfile or lockfile in a format that wasn't workable for the user, they wouldn't be able to resolve any packages from that index and so I think an issue would already have been raised. That being said, we can add additional validations/coercing/error reporting if determined its currently lacking in this area.

@jeffwidman
Copy link
Member Author

jeffwidman commented Aug 10, 2023

if the source URL were in the Pipfile or lockfile in a format that wasn't workable for the user, they wouldn't be able to resolve any packages from that index

👍 Yeah that was my thought too... this method only pulls sources from the Pipfile itself, it's not looking at what :dependabot: injects. And typically our first debugging step is tell someone "Did you try running the package manager without :dependabot: ? What was the result?" So they should quickly observe the failure...

In fact the opposite would be worse! If the native package manager (in this case pipenv) is failing for a URL, but :dependabot: works because we silently munge it then that'd lead to confusing/difficult to debug scenario.

@jeffwidman jeffwidman force-pushed the stop-coercing-pipfile-source-urls branch from edd027f to 0be5778 Compare August 10, 2023 16:12
I had to stare at this code golf for a little bit to realize that it's enforcing that every source URL in a `Pipfile` has one-and-only-one trailing slash.

I started to tweak it a little to make it more readable, but I decided instead that this is business we really shouldn't be in... Dependabot is the tool that runs the package manager, not the package manager itself.

So if a package manager doesn't like a URL that lacks a trailing slash, it should be the one handling the coercing or raising the error, and then we transparently forward that to the user.

There are some use cases where we need to handle URL normalization for deduping purposes within Dependabot internals, but I poked around a bit and as far as I can tell they don't apply here.

So let's trust the user / package managers to do the right thing (which may mean giving us a clear error).
@jeffwidman jeffwidman force-pushed the stop-coercing-pipfile-source-urls branch from 0be5778 to 1ffc528 Compare August 10, 2023 16:49
@jeffwidman jeffwidman enabled auto-merge (squash) August 10, 2023 16:50
@jeffwidman jeffwidman merged commit 82388fd into main Aug 10, 2023
@jeffwidman jeffwidman deleted the stop-coercing-pipfile-source-urls branch August 10, 2023 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants