-
Notifications
You must be signed in to change notification settings - Fork 73
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
Throw an error if Devfile from unsupported ssh repository is not resolved #763
Conversation
5a74010
to
03bbc4a
Compare
/retest |
/retest |
@@ -73,7 +73,7 @@ public Optional<String> filename() { | |||
|
|||
@Override | |||
public String location() { | |||
return devfileFilename; | |||
return String.format("https://%s/%s/%s", hostName, repository, devfileFilename); |
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.
Sounds like significant changes.
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.
I do not think so. As I mentioned in the description this function is used to handle unsupported git provider and this url pattern is just potentially can match a RAW devfile url. If we handle a known provider, we have the pattern to fetch the devfile, but here we just trying to guess and with this pattern it is more likely that we will fetch the devfile.
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.
would it be possible to add javadoc? it is not intuitive why location for GitSshUrl returns https?
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.
This change is not related to this pull request so I decided to revert it and leave a todo.
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.
If this PR is fixing an issue eclipse-che/che#23277 , then workspace should start successfully without any errors following test scenario described by @ibuziuk in his comment eclipse-che/che#23277 (comment)
- SSH key is configured
- workspace started via private SSH URL
- workspace is started correctly based on the devfile from the private SSH repo.
N.B. no OAuth / PAT are configured
Otherwise it looks like a regression compare to previous version, IMHO
If PR is fixing specific use case, I would suggest opening separate issue with clear description.
@dmytro-ndp A typical example is when it's not GitHub, GitLab, Bitbucket or Azure DevOps repository or when SSH URL is used. The pull request fixes the case when an SSH url from an unsupported provider is used, which is mentioned in the description above. |
/retest |
@vinokurig: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
@vinokurig : thank you for the answer! If I understood the following description of issue #22025 correctly, an SSH URL from an unsupported provider should work in Eclipse Che:
As far as I remember, I was able to create and open a workspace from a Gogs Git repository using a GIT+SSH link (ssh://git@ec2-3-147-242-143.us-east-2.compute.amazonaws.com:10022/test/quarkus-api-example-public.git) in Eclipse Che 7.98.x, and the devfile from the Git repository was successfully applied. My concern is that, to avoid regression, it makes sense to ensure that Eclipse Che continues to support this functionality after this PR is merged. For example, someone could create a PR for the Dashboard to properly handle 500 server errors from Che Server in case of SSH url from unsupported provider like Gogs, and merge it together with this PR. |
@dmytro-ndp are we good to backport it to 7.98.x in order to properly resolve the SSH flow? |
@vinokurig : thank you for clarifying the PR test scenario and for opening a dedicated issue to describe the fix use case. The PR has been successfully tested using Eclipse Che Next with che-server quay.io/eclipse/che-server:pr-763 Test scenario:Preparation:
Test creation of workspace using correct Gogs server SSH URL
Test creation of workspace using incorrect Gogs server SSH URLWorkspace created successfully without warning message Screencastscreen-capture.5.webm@ibuziuk: +1 to backport this PR to 7.98.x branch in order to properly resolve the SSH flow. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dmytro-ndp, ibuziuk, 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 |
…lved (#763) Throw an error if Devfile from unsupported ssh repository is not resolved instead of returning an empty factory dto without devfile. The ApiException will be handled by dashboard and default devfile will be used: https://github.com/eclipse-che/che-dashboard/blob/05bf4383a45cb367924c788763beb5c7689a7c79/packages/dashboard-frontend/src/components/WorkspaceProgress/CreatingSteps/Fetch/Devfile/index.tsx#L215
Build 3.20 :: server_3.x/385: Console, Changes, Git Data |
Build 3.20 :: sync-to-downstream_3.x/8838: Console, Changes, Git Data |
Build 3.20 :: push-latest-container-to-quay_3.x/5104: Console, Changes, Git Data |
Build 3.20 :: get-sources-rhpkg-container-build_3.x/8957: server : 3.x :: Build 66813515 : quay.io/devspaces/server-rhel9:3.20-3 |
Build 3.20 :: server_3.x/385: Upstream sync done; /DS_CI/sync-to-downstream_3.x/8838 triggered |
Build 3.20 :: update-digests_3.x/8680: Console, Changes, Git Data |
…lved (#763) Throw an error if Devfile from unsupported ssh repository is not resolved instead of returning an empty factory dto without devfile. The ApiException will be handled by dashboard and default devfile will be used: https://github.com/eclipse-che/che-dashboard/blob/05bf4383a45cb367924c788763beb5c7689a7c79/packages/dashboard-frontend/src/components/WorkspaceProgress/CreatingSteps/Fetch/Devfile/index.tsx#L215
…lved (#763) Throw an error if Devfile from unsupported ssh repository is not resolved instead of returning an empty factory dto without devfile. The ApiException will be handled by dashboard and default devfile will be used: https://github.com/eclipse-che/che-dashboard/blob/05bf4383a45cb367924c788763beb5c7689a7c79/packages/dashboard-frontend/src/components/WorkspaceProgress/CreatingSteps/Fetch/Devfile/index.tsx#L215
…lved (#763) (#770) Throw an error if Devfile from unsupported ssh repository is not resolved instead of returning an empty factory dto without devfile. The ApiException will be handled by dashboard and default devfile will be used: https://github.com/eclipse-che/che-dashboard/blob/05bf4383a45cb367924c788763beb5c7689a7c79/packages/dashboard-frontend/src/components/WorkspaceProgress/CreatingSteps/Fetch/Devfile/index.tsx#L215
What does this PR do?
Throw an error if Devfile from unsupported ssh repository is not resolved instead of returning an empty factory dto without devfile. The
ApiException
will be handled by dashboard and default devfile will be used: https://github.com/eclipse-che/che-dashboard/blob/05bf4383a45cb367924c788763beb5c7689a7c79/packages/dashboard-frontend/src/components/WorkspaceProgress/CreatingSteps/Fetch/Devfile/index.tsx#L215Screenshot/screencast of this PR
What issues does this PR fix or reference?
fixes eclipse-che/che#23364
How to test this PR?
quay.io/eclipse/che-server:pr-763
git@gitea.com:ivinokur/test.git
See workspace starts from the devfile located in the repository.
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
completedRelease Notes
Reviewers
Reviewers, please comment how you tested the PR when approving it.