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

fix(git-url): clean up state of parsed URL #24306

Merged
merged 6 commits into from
Sep 14, 2023

Conversation

lcobucci
Copy link
Contributor

@lcobucci lcobucci commented Sep 7, 2023

Changes

This ensures the state of parsedUrl doesn't have authentication credentials prior to appending the authentication tokens.

Context

This is meant to verify and solve the issue described here.

Documentation

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

This ensures the state of `parsedUrl` doesn't have authentication
credentials prior to appending the authentication tokens.
lib/util/git/url.spec.ts Outdated Show resolved Hide resolved
lib/util/git/url.spec.ts Outdated Show resolved Hide resolved
lib/util/git/url.ts Show resolved Hide resolved
We were missing the path in the URL to ensure the test behaves as expected.

Co-authored-by: Tobias <github@tobiasgabriel.de>
lib/util/git/url.ts Show resolved Hide resolved
lib/util/git/url.ts Outdated Show resolved Hide resolved
The library doesn't allow us to update `password`, which is covered by `token` anyway.

Co-authored-by: Tobias <github@tobiasgabriel.de>
@lcobucci
Copy link
Contributor Author

lcobucci commented Sep 8, 2023

@Shegox it should be good now :)

@lcobucci
Copy link
Contributor Author

lcobucci commented Sep 9, 2023

@Shegox do you know if the broken tests are related to real expectations?

@Shegox
Copy link
Contributor

Shegox commented Sep 11, 2023

I'm not sure, I haven't seen the syntax with the username being part of the hostRule in the wild.
As the test was added by @lfcyja in #23354, maybe they can add some insights. Thanks!

Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

needs prettier fix

@lcobucci
Copy link
Contributor Author

I'm not sure, I haven't seen the syntax with the username being part of the hostRule in the wild. As the test was added by @lfcyja in #23354, maybe they can add some insights. Thanks!

Looking at the patch, it doesn't seem strictly related - more like side effects of the status quo. @Shegox should we move forward with this? If so, I'll fix the test expectations and address the coding style issues.

Signed-off-by: Luís Cobucci <lcobucci@gmail.com>
Signed-off-by: Luís Cobucci <lcobucci@gmail.com>
@lcobucci lcobucci requested review from Shegox and viceice September 12, 2023 18:17
Copy link
Contributor

@Shegox Shegox left a comment

Choose a reason for hiding this comment

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

Looks good and for GitHub and GitLab this should work as expected. For the other platforms I personally don't see a reason why the "user" should be part of the URL in the user@domain syntax.

@rarkins @viceice could you please take a look at this PR and how we should proceed. Thanks!

@viceice viceice added this pull request to the merge queue Sep 14, 2023
Merged via the queue into renovatebot:main with commit 0b50feb Sep 14, 2023
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 36.93.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

@lcobucci lcobucci deleted the fix-url-generation branch September 14, 2023 08:11
@@ -77,6 +77,14 @@ describe('util/git/url', () => {
'https://x-access-token:token@github.com/some/repo'

Choose a reason for hiding this comment

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

Wyd

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants