Respect the ScmComunicationException on token validation step #715

vinokurig · 2024-08-30T12:37:52Z

What does this PR do?

Throw the ScmCommunicationException on token validation step instead of returning invalid result.
Propagate the ScmCommunicationException to the dashboard as an ApiException.
Throw ScmUnauthorizedException on each scm api request with invalid token.
Log error instead of throwing RuntimeException on workspace provision steps.

Screenshot/screencast of this PR

What issues does this PR fix or reference?

eclipse-che/che#23076

How to test this PR?

Deploy Che from the PR image: quay.io/ivinokur/che-server:che-23076 The image is built with a modification: to simulate the unreachable SCM server I changed the api endpoint url in the GitHub api client:

che-server/wsmaster/che-core-api-factory-github-common/src/main/java/org/eclipse/che/api/factory/server/github/GithubApiClient.java

Line 57 in e04d787

public static final String GITHUB_SAAS_ENDPOINT_API = "https://api.github.com";
Configure Che to use dashboard image from the related pull request: quay.io/eclipse/che-dashboard:pr-1179
Configure GitHub oauth
Start a workspace from a GitHub repository with devfile.

See: a warning notification appears during workspace startup:

The workspace starts successfully.

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 · 2024-09-03T11:53:10Z

/retest

artaleks9 · 2024-09-05T11:20:39Z

By some reason, the changes in test file are not staged.
Maybe there is changed UI, or something like that.
I investigate it.

artaleks9 · 2024-09-06T14:59:02Z

@vinokurig
The ci/prow/v14-gitlab-with-oauth-setup-flow is fail due to changes in IDE
I provided the screenshots to compare and I'm going to fix up it in our Che test repository
The functionality related to git provider OAuth works properly. I've checked it manually using quay.io/eclipse/che-server:pr-715

artaleks9 · 2024-09-06T15:01:21Z

So, there are no restrictions related tests to merge PR.

tolusha · 2024-09-09T09:47:05Z

...orkspace/infrastructure/kubernetes/namespace/configurator/CredentialsSecretConfigurator.java

@@ -79,7 +83,7 @@ public void configure(NamespaceResolutionContext namespaceResolutionContext, Str
                  | ScmConfigurationPersistenceException
                  | UnsatisfiedScmPreconditionException
                  | ScmUnauthorizedException e) {
-                throw new RuntimeException(e);
+                LOG.error(e.getMessage(), e);


What is the benefits of logging error?

We do not interrupt the workspace start in this case.

tolusha · 2024-09-09T09:48:27Z

...bitbucket/src/main/java/org/eclipse/che/api/factory/server/bitbucket/BitbucketApiClient.java

@@ -228,6 +235,8 @@ private <T> T executeRequest(
            throw new ScmBadRequestException(body);
          case HTTP_NOT_FOUND:
            throw new ScmItemNotFoundException(body);
+          case HTTP_UNAUTHORIZED:
+            throw new ScmUnauthorizedException(body, "github", "v1", "");


Can we move constant somewhere else?
Copy/paste from github?

Copy/paste from github?

right, fixed

Can we move constant somewhere else?

Could you please elaborate on this?

tolusha · 2024-09-09T09:49:57Z

...y-github-common/src/main/java/org/eclipse/che/api/factory/server/github/GithubApiClient.java

@@ -290,6 +296,8 @@ private <T> T executeRequest(
            throw new ScmBadRequestException(body);
          case HTTP_NOT_FOUND:
            throw new ScmItemNotFoundException(body);
+          case HTTP_UNAUTHORIZED:
+            throw new ScmUnauthorizedException(body, "github", "v1", "");


Changed to v2 corresponds to oauth version.

tolusha · 2024-09-09T09:50:13Z

...-factory-gitlab/src/main/java/org/eclipse/che/api/factory/server/gitlab/GitlabApiClient.java

@@ -159,6 +163,8 @@ private <T> T executeRequest(
            throw new ScmBadRequestException(body);
          case HTTP_NOT_FOUND:
            throw new ScmItemNotFoundException(body);
+          case HTTP_UNAUTHORIZED:
+            throw new ScmUnauthorizedException(body, "github", "v1", "");


copy/paste from github ?

ibuziuk

@vinokurig great job, but can we make UX more informative - Error message should say that Github might be down + add a link to the statuspage - https://www.githubstatus.com/

vinokurig · 2024-09-11T12:55:15Z

@ibuziuk

can we make UX more informative - Error message should say that Github might be down + add a link to the statuspage - https://www.githubstatus.com/

I've improved the notification text but unfortunately there is no quick way to have the link clickable in the warning notification, would that be ok in the scope of the issue? cc @olexii4 @akurinnoy

ibuziuk · 2024-09-12T08:28:03Z

We should either make the link clickable or just reword the warning without the link - GitHub might not be operational. Check the status page.

vinokurig · 2024-09-12T13:00:18Z

@ibuziuk I have removed the links from the notification and opened an issue to be able to add links

vinokurig · 2024-09-13T08:03:19Z

/retest

openshift-ci · 2024-09-13T09:07:54Z

@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	`deda774`	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.

openshift-ci · 2024-09-13T13:57:22Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: artaleks9, 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:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

devstudio-release · 2024-09-13T13:58:22Z

Build 3.17 :: server_3.x/359: Console, Changes, Git Data

devstudio-release · 2024-09-13T14:03:12Z

Build 3.17 :: sync-to-downstream_3.x/7701: Console, Changes, Git Data

devstudio-release · 2024-09-13T14:03:38Z

Build 3.17 :: get-sources-rhpkg-container-build_3.x/7693: Console, Changes, Git Data

devstudio-release · 2024-09-13T14:06:07Z

Build 3.17 :: get-sources-rhpkg-container-build_3.x/7693:

server : 3.x :: Failed in 64203004 : BREW:BUILD/STATUS:UNKNOWN
FAILURE:; copied to quay

vinokurig requested review from SDawley, ibuziuk and tolusha as code owners August 30, 2024 12:37

vinokurig mentioned this pull request Aug 30, 2024

Show a warning notification on fetch token error when starting a work… eclipse-che/che-dashboard#1179

Merged

vinokurig force-pushed the che-23076 branch 2 times, most recently from 99a8bc0 to a914f58 Compare August 30, 2024 13:56

vinokurig marked this pull request as draft August 30, 2024 14:17

openshift-ci bot added the do-not-merge/work-in-progress label Aug 30, 2024

Intercept ScmComunicationException on workspace start

46114a4

vinokurig force-pushed the che-23076 branch from a914f58 to 46114a4 Compare September 2, 2024 14:40

vinokurig marked this pull request as ready for review September 3, 2024 10:44

openshift-ci bot removed the do-not-merge/work-in-progress label Sep 3, 2024

tolusha reviewed Sep 9, 2024

View reviewed changes

ibuziuk reviewed Sep 9, 2024

View reviewed changes

fixup! Intercept ScmComunicationException on workspace start

520e887

tolusha approved these changes Sep 9, 2024

View reviewed changes

openshift-ci bot assigned tolusha Sep 9, 2024

openshift-ci bot added the lgtm label Sep 9, 2024

openshift-merge-robot added the needs-rebase label Sep 11, 2024

Merge branch 'main' of github.com:eclipse-che/che-server into che-23076

6f1997f

openshift-ci bot removed the lgtm label Sep 11, 2024

openshift-merge-robot removed the needs-rebase label Sep 11, 2024

fixup! Intercept ScmComunicationException on workspace start

1b4b316

vinokurig force-pushed the che-23076 branch 3 times, most recently from 76f172d to 9d496cb Compare September 11, 2024 12:37

fixup! Intercept ScmComunicationException on workspace start

deda774

vinokurig force-pushed the che-23076 branch from 9d496cb to deda774 Compare September 11, 2024 12:52

vinokurig mentioned this pull request Sep 12, 2024

Add an ability to pass links to dashboard notifications. eclipse-che/che#23142

Open

ibuziuk approved these changes Sep 12, 2024

View reviewed changes

openshift-ci bot assigned ibuziuk Sep 12, 2024

openshift-ci bot added the lgtm label Sep 12, 2024

artaleks9 approved these changes Sep 13, 2024

View reviewed changes

openshift-ci bot assigned artaleks9 Sep 13, 2024

vinokurig merged commit 6121661 into main Sep 13, 2024
27 of 28 checks passed

vinokurig deleted the che-23076 branch September 13, 2024 13:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect the ScmComunicationException on token validation step #715

Respect the ScmComunicationException on token validation step #715

vinokurig commented Aug 30, 2024 •

edited

Loading

vinokurig commented Sep 3, 2024

artaleks9 commented Sep 5, 2024

artaleks9 commented Sep 6, 2024

artaleks9 commented Sep 6, 2024

tolusha Sep 9, 2024

vinokurig Sep 9, 2024

tolusha Sep 9, 2024

vinokurig Sep 9, 2024

tolusha Sep 9, 2024

vinokurig Sep 9, 2024

tolusha Sep 9, 2024

vinokurig Sep 9, 2024

ibuziuk left a comment

vinokurig commented Sep 11, 2024

ibuziuk commented Sep 12, 2024

vinokurig commented Sep 12, 2024 •

edited

Loading

vinokurig commented Sep 13, 2024

openshift-ci bot commented Sep 13, 2024

openshift-ci bot commented Sep 13, 2024

devstudio-release commented Sep 13, 2024

devstudio-release commented Sep 13, 2024

devstudio-release commented Sep 13, 2024

devstudio-release commented Sep 13, 2024

Respect the ScmComunicationException on token validation step #715

Respect the ScmComunicationException on token validation step #715

Conversation

vinokurig commented Aug 30, 2024 • edited Loading

What does this PR do?

Screenshot/screencast of this PR

What issues does this PR fix or reference?

How to test this PR?

PR Checklist

Release Notes

Reviewers

vinokurig commented Sep 3, 2024

artaleks9 commented Sep 5, 2024

artaleks9 commented Sep 6, 2024

artaleks9 commented Sep 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibuziuk left a comment

Choose a reason for hiding this comment

vinokurig commented Sep 11, 2024

ibuziuk commented Sep 12, 2024

vinokurig commented Sep 12, 2024 • edited Loading

vinokurig commented Sep 13, 2024

openshift-ci bot commented Sep 13, 2024

openshift-ci bot commented Sep 13, 2024

devstudio-release commented Sep 13, 2024

devstudio-release commented Sep 13, 2024

devstudio-release commented Sep 13, 2024

devstudio-release commented Sep 13, 2024

vinokurig commented Aug 30, 2024 •

edited

Loading

vinokurig commented Sep 12, 2024 •

edited

Loading