fix: support multiple URL protocols #76
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These aren't strictly "URLs" in the web sense. They may be a variety of formats and don't strictly conform to HTTP-style URLs, like SSH style URLs. This change aligns the validation with Git's own URL validation. While the validation doesn't necessarily gaurantee that the URL is actually valid, calling the checkout action (which would be necessarily to use this action anyway) would fail before this if it couldn't clone a submodule due to a bad URL. - Switched from `.url()` to `.regex()` string validator using the regex present in url.c[1] in Git's source code. - Added unit test and test fixture to check compatibility with SSH-style URLs. [1]: https://github.com/git/git/blob/77bd3ea9f54f1584147b594abc04c26ca516d987/url.c#L6-L12
Since the action no longer accepts HTTP-style URLs, we need to account for more cases. This commit adds a function that attempts a "best guess" remote name extraction. It looks for specific characters in the URL that probably demarcate the remote name. It's not a perfect solution, but it works well in the vast majority of cases. - Added `getRemoteName` function to get remote name. - Added unit tests with a bunch of example cases that cover a gamut of different URL formats. The test cases were taken from this Stack Overflow post: https://stackoverflow.com/questions/31801271/what-are-the-supported-git-url-formats
sgoudham
changed the title
sgoudham
approved these changes
Apr 1, 2025
Owner
sgoudham
left a comment
sgoudham
left a comment
There was a problem hiding this comment.
Sorry for the wait on this.
I was quite hesitant on accepting the changes to the remoteName logic but it has an extensive test suite which I'm happy with.
I've made some nitpicky modifications to the test setup so hope you don't mind that. All in all, it looks good! Thanks for taking the time to contribute, really appreciated!
Owner
|
Ignore me, totally forgot that GitHub Actions need to publish their compiled code into |
sgoudham
added a commit
that referenced
this pull request
Apr 2, 2025
* Loosened validation of submodule URL. These aren't strictly "URLs" in the web sense. They may be a variety of formats and don't strictly conform to HTTP-style URLs, like SSH style URLs. This change aligns the validation with Git's own URL validation. While the validation doesn't necessarily gaurantee that the URL is actually valid, calling the checkout action (which would be necessarily to use this action anyway) would fail before this if it couldn't clone a submodule due to a bad URL. - Switched from `.url()` to `.regex()` string validator using the regex present in url.c[1] in Git's source code. - Added unit test and test fixture to check compatibility with SSH-style URLs. [1]: https://github.com/git/git/blob/77bd3ea9f54f1584147b594abc04c26ca516d987/url.c#L6-L12 * Improved remote name extraction. Since the action no longer accepts HTTP-style URLs, we need to account for more cases. This commit adds a function that attempts a "best guess" remote name extraction. It looks for specific characters in the URL that probably demarcate the remote name. It's not a perfect solution, but it works well in the vast majority of cases. - Added `getRemoteName` function to get remote name. - Added unit tests with a bunch of example cases that cover a gamut of different URL formats. The test cases were taken from this Stack Overflow post: https://stackoverflow.com/questions/31801271/what-are-the-supported-git-url-formats * chore: delete helper function & rename test --------- Co-authored-by: sgoudham <sgoudham@gmail.com>
Contributor
Author
|
No problem. I realize that was a pretty complicated change, but the old logic didn't really work once other URL formats get introduced. It's not something I had anticipated which is why I added a bunch of tests for it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously this action was limited to HTTP-style URLs (e.g.
https://example.com/repo.git), but Git supports a variety of URL schemas across different protocols, like SSH, a popular alternative method supported by Github. This PR seeks to allow this action to work on URLs in.gitmoduleswith many different URL formats.This PR is a follow on from #74. It replaces the existing validation that strictly validates HTTP-style URLs with the same logic that Git uses to validate URLs.
Additionally, because URLs no longer follow a strict format, the logic used to extract the remote name was changed. Due to URLs now being so varied, it uses a more dynamic, "best guess" method of identifying the remote name. This component has been tested fairly extensively and it passed all preexisting tests, but it's worth calling out as it is a non-trivial logic change.