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

Throw an error if Devfile from unsupported ssh repository is not resolved #763

Merged
merged 2 commits into from
Feb 26, 2025

Conversation

vinokurig
Copy link
Contributor

@vinokurig vinokurig commented Feb 13, 2025

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#L215

Screenshot/screencast of this PR

What issues does this PR fix or reference?

fixes eclipse-che/che#23364

How to test this PR?

  1. Deploy che with the pull request image: quay.io/eclipse/che-server:pr-763
  2. Create and add an SSH key to Gitea and Che user namespace.
  3. Start workspcae from Gitea ssh url e.g. 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:

Release Notes

Reviewers

Reviewers, please comment how you tested the PR when approving it.

@vinokurig vinokurig force-pushed the che-23277 branch 2 times, most recently from 5a74010 to 03bbc4a Compare February 13, 2025 15:34
@vinokurig
Copy link
Contributor Author

/retest

@vinokurig
Copy link
Contributor Author

/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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like significant changes.

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@dmytro-ndp dmytro-ndp left a 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.

@vinokurig
Copy link
Contributor Author

@dmytro-ndp
Why do we need a separate issue for the case described in the pull request? From the current issue description:

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.
But if you insist, I can create a separate issue.

@artaleks9
Copy link
Contributor

/retest

Copy link

openshift-ci bot commented Feb 18, 2025

@vinokurig: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/v14-gitlab-with-oauth-setup-flow 5715f7e link true /test v14-gitlab-with-oauth-setup-flow

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.

@dmytro-ndp
Copy link
Contributor

@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:

Through the GIT+SSH protocol Che would be able to fully support any git service. That won't replace the HTTP + OAuth flow that remains the preferred if support (GitHub, BB, GitLab and Azure DevOps), but an alternative that allows Che to work on other services too (gerrit, gogs, githut...).

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.

@ibuziuk
Copy link
Member

ibuziuk commented Feb 24, 2025

@dmytro-ndp are we good to backport it to 7.98.x in order to properly resolve the SSH flow?

@dmytro-ndp
Copy link
Contributor

@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:

  1. Create private repo with devfile in Gogs server.
  2. Add public SSH key to Gogs server.
  3. Open Eclipse Che and add SSH key to User Preferences in User Dashboard.

Test creation of workspace using correct Gogs server SSH URL

  • Workspace created successfully.
  • Project cloned correctly.
  • Devfile from the git repo has been applied.

Test creation of workspace using incorrect Gogs server SSH URL

Workspace created successfully without warning message The specified link does not contain any Devfile. Continue with default devfile?.

Screencast

screen-capture.5.webm

@ibuziuk: +1 to backport this PR to 7.98.x branch in order to properly resolve the SSH flow.

Copy link

openshift-ci bot commented Feb 25, 2025

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@vinokurig vinokurig merged commit 27505d6 into main Feb 26, 2025
27 of 28 checks passed
@vinokurig vinokurig deleted the che-23277 branch February 26, 2025 09:19
vinokurig added a commit that referenced this pull request Feb 26, 2025
…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
@devspacesbuild
Copy link

Build 3.20 :: server_3.x/385: Console, Changes, Git Data

@devspacesbuild
Copy link

@devspacesbuild
Copy link

@devspacesbuild
Copy link

@devspacesbuild
Copy link

@devspacesbuild
Copy link

Build 3.20 :: server_3.x/385: SUCCESS

Upstream sync done; /DS_CI/sync-to-downstream_3.x/8838 triggered

@devspacesbuild
Copy link

vinokurig added a commit that referenced this pull request Feb 26, 2025
…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
vinokurig added a commit that referenced this pull request Feb 26, 2025
…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
vinokurig added a commit that referenced this pull request Feb 26, 2025
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Start workspace from Gitea SSH url is interrupted by a warning page
6 participants