Skip to content

fix: downloading github url with / in branch name#430

Open
cwyl02 wants to merge 1 commit intohashicorp:mainfrom
cwyl02:main
Open

fix: downloading github url with / in branch name#430
cwyl02 wants to merge 1 commit intohashicorp:mainfrom
cwyl02:main

Conversation

@cwyl02
Copy link

@cwyl02 cwyl02 commented Apr 11, 2023

This fixes #422

@cwyl02
Copy link
Author

cwyl02 commented Apr 11, 2023

Hi @claire-labry @picatz @nywilken, please take a look. We use go-getter internally and merging this will fix #422 thus help getting our internal CI passing.

@cwyl02
Copy link
Author

cwyl02 commented Apr 11, 2023

also curious if main lands to 1.x release or 2.x releases?

@hashicorp-cla
Copy link

hashicorp-cla commented Dec 29, 2023

CLA assistant check
All committers have signed the CLA.

@cwyl02
Copy link
Author

cwyl02 commented Jun 17, 2024

any updates on this PR?

Copy link
Contributor

@CreatorHead CreatorHead left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, the overall approach makes sense.
Splitting the source on ? before path parsing correctly fixes the issue where / inside query parameters (e.g. branch names like feature/foo) were being misinterpreted as part of the repo path.

I have one small concern before merging:

Handling multiple ? characters
Right now the code errors if strings.Split(src, "?") results in more than two parts. In practice, only the first ? is a delimiter in a URL; additional ? characters can legally appear inside the query string as data. Previously, these cases would likely have worked, so this introduces a stricter behavior than before.

Would it make sense to switch to strings.SplitN(src, "?", 2) and drop the len(parts) > 2 check? That would preserve the fix while avoiding a potential regression.

Tests
The added test is helpful. It would be great to also include a case that mirrors the original failure more directly, e.g. a URL with ref=feature/foo (optionally with a subdirectory as well), to clearly lock in the intended behavior.

With those tweaks, I think this change would be in good shape to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GitHub https downloads break when there is a / in the branch name.

3 participants

Comments