-
Notifications
You must be signed in to change notification settings - Fork 70
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
Support SSH urls #539
Support SSH urls #539
Conversation
Signed-off-by: Igor Vinokur <[email protected]>
@vinokurig could you please update the description e.g. ssh setup step and provide an image for testing |
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.
Tested for Azure DevOps
...ure-devops/src/main/java/org/eclipse/che/api/factory/server/azure/devops/AzureDevOpsUrl.java
Outdated
Show resolved
Hide resolved
@@ -80,7 +80,7 @@ private String fetchContent( | |||
String authorization; | |||
if (isNullOrEmpty(credentials)) { | |||
PersonalAccessToken token = | |||
personalAccessTokenManager.get(remoteFactoryUrl.getHostName()); | |||
personalAccessTokenManager.get(remoteFactoryUrl.getProviderUrl()); |
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.
Will it affect existed PATs?
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.
It won't. This is just a refactoring of a method which retrieves SCM endpoint. The output is the same but the method is different.
test-project (main) $ git remote -v
origin [email protected]:v3/my-abazko/test-project/test-project (fetch)
origin [email protected]:v3/my-abazko/test-project/test-project (push)
test-project (main) $ touch test-file
test-project (main) $ git add -A
test-project (main) $ git commit -m "Add file" --signoff
[main 3205e4c] Add file
1 file changed, 0 insertions(+), 0 deletions(-)
create mode 100644 test-file
test-project (main) $ git push origin
Warning: Permanently added 'ssh.dev.azure.com,40.74.28.27' (RSA) to the list of known hosts.
Enumerating objects: 4, done.
Counting objects: 100% (4/4), done.
Delta compression using up to 4 threads
Compressing objects: 100% (2/2), done.
Writing objects: 100% (3/3), 328 bytes | 109.00 KiB/s, done.
Total 3 (delta 0), reused 0 (delta 0), pack-reused 0
remote: Analyzing objects... (3/3) (28 ms)
remote: Storing packfile... done (63 ms)
remote: Storing index... done (60 ms)
To ssh.dev.azure.com:v3/my-abazko/test-project/test-project
dec2ba5..3205e4c main -> main
test-project (main) $ |
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.
Tested the following scenarios with private GitHub repo [email protected]:ibuziuk/private.git
- vanilla installation, no OAuth, PAT, SSH is setup - workspace is failing to start with
-
only SSH is setup (no PAT / OAuth) - same error as above
-
SSH and PAT are set
devfile is found, workspace is started and the project is cloned + remotes are set correctly:
origin [email protected]:ibuziuk/private.git (fetch)
origin [email protected]:ibuziuk/private.git (push)
Basically, we need to document that SSH setup is not enough to make the flow work (PAT or OAuth should be also set). Also, wondering if we can make the error more explicit
@ScrewTSW when you will be doing review please verify against bitbucket |
The error is thrown because Che could not find GitHub oauth provider in its list of registered providers. The same error appears when a private https url is used without Oauth and PAT being configured. This question worth a separate discussion. |
sounds, good |
LGTM, verified using the prcheck image - AOK |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ibuziuk, ScrewTSW, tolusha, vinokurig 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 |
Build 3.9 :: server_3.x/203: Console, Changes, Git Data |
Build 3.9 :: sync-to-downstream_3.x/4230: Console, Changes, Git Data |
Build 3.9 :: get-sources-rhpkg-container-build_3.x/4085: server : 3.x :: Failed in 54904601 : BREW:BUILD/STATUS:UNKNOWN |
After this SSH URLs other than GitHub/GitLab/BB/Azure don't work at all. It doesn't matter if the repo is public or private, the workspace will fail to start. This is a regression from what we had before because the workspace could start although the devfile was ignored. (I have tried with sourcehut for instance and I verified that it fails at the step "Inspecting repo for a devfile" with message "Cannot build factory with any of the provided parameters. Please check parameters correctness, and resend query."). Moreover, as mentioned by @ibuziuk above, for GitHub/GitLab/BB/Azure SSH URLs it still requires a PAT secret (beyond the SSH private key) and there is no obvious value using the SSH URL. But I may be wrong here because using SSH key to pull/push rather then PAT can avoid problems with token expiration (to verify) so that, at least, have some value for users. |
filed a bug report: eclipse-che/che#22473 |
What does this PR do?
Extend each SCM provider implementation to support SSH urls.
Screenshot/screencast of this PR
What issues does this PR fix or reference?
fixes eclipse-che/che#22354
How to test this PR?
quay.io/eclipse/che-server:pr-539
See:
PR Checklist
As the author of this Pull Request I made sure that:
What issues does this PR fix or reference
andHow to test this PR
completedReviewers
Reviewers, please comment how you tested the PR when approving it.