-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Properly handle provider host name when using http(s) prefix when adding a new integration #7831
Properly handle provider host name when using http(s) prefix when adding a new integration #7831
Conversation
Thanks for contributing, @Shulammite-Aso! 🍊 /werft run 👍 started the job as gitpod-build-verify-provider-hostname-fork.0 |
Codecov Report
@@ Coverage Diff @@
## main #7831 +/- ##
=========================================
+ Coverage 8.62% 10.20% +1.57%
=========================================
Files 33 18 -15
Lines 2341 1009 -1332
=========================================
- Hits 202 103 -99
+ Misses 2135 905 -1230
+ Partials 4 1 -3
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
@Shulammite-Aso You'll need to sign a Contributor License Agreement (CLA)[1] once before merging your first contribution. Looping in @meysholdt to reach out about the CLA. 🏓 |
@meysholdt how do i sign the Contributor license agreement? I followed this link provided in the docs here to see if it provides hints on how to sign it, but it appears the link is broken. |
cc @meysholdt |
Hi @Shulammite-Aso, I've sent the CLA via email / DocuSign. |
Hi @meysholdt i just signed the document. |
Hi @Shulammite-Aso! Awesome, thank you for signing! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the change. I've proposed a few changes.
use 'startsWith' method to strip off http(s) from provider hostname
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Associated issue: #7492 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Shulammite-Aso thanks for the contribution!
I left some comments. The main issue remains is, that Gitpod won't accept http
URLs for provider connections on the server side.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
/werft run 👍 started the job as gitpod-build-verify-provider-hostname-fork.1 |
Let's verify with a preview environment and lend this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution, @Shulammite-Aso!
🎉 🎉 🎉
Hi @gtsiolis this PR is approved, is it now ready to get merged? |
Hey, @Shulammite-Aso! I wanted to confirm internally that the signed CLA is in place and stored for future contributions. Let's merge this. 🚀 Thanks @AlexTugarev @JanKoehnlein @meysholdt @csweichel for helping move this forward! 🏀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@JanKoehnlein could you approve this to unblock merging[1]? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks a lot for this contribution
Description
When a user has manually included http(s) prefixes in their provider hostname, strip it off so that it uses only the gitpod provided prefix in the configured URL and Redirect URL.
Related Issue(s)
Fixes #7492
How to test
Go to dashboard and under integrations try to add new integration with a self-hosted instance of a provider, then notice that any HTTP(s) prefix is removed from the configured URL
Release Notes
Documentation