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] Perforce auth provider panics when only IP is provided #64533

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

pjlast
Copy link
Contributor

@pjlast pjlast commented Aug 19, 2024

When only an IP address and port (like 127.0.0.1:3080 is provided as p4.port, the auth provider will panic since the value can't be parsed as a URL correctly. This is common to do when talking to Perforce, because the p4 CLI automatically prefixes tcp: if this is the case.

However, we can't quite do this because the auth provider URL and the code host URL need to match, so if we auto-adjust the auth provider URL they will no longer match.

This PR adds error handling on the auth provider, and will display a more helpful error message instead of panicking.

image

Test plan

Unit test added

Changelog

@cla-bot cla-bot bot added the cla-signed label Aug 19, 2024
@pjlast pjlast requested a review from a team August 19, 2024 11:27
@github-actions github-actions bot added team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all labels Aug 19, 2024
Copy link
Contributor

@Rhia2 Rhia2 left a comment

Choose a reason for hiding this comment

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

This LGTM. Thank you

@mmanela
Copy link
Contributor

mmanela commented Aug 19, 2024

Should we clarify this in our docs too?

@pjlast pjlast enabled auto-merge (squash) August 19, 2024 12:46
@pjlast pjlast disabled auto-merge August 19, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants