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

Do not throw an error on token refresh when starting a workspace #609

Closed
wants to merge 1 commit into from

Conversation

vinokurig
Copy link
Contributor

@vinokurig vinokurig commented Nov 14, 2023

What does this PR do?

Intercept exceptions in the token/refresh method of the FactoryService in order to not to interrupt the workspace start flow if something wrong happens on the token refresh step. For example the workspace project is public and no corresponding oauth is configured.

Screenshot/screencast of this PR

What issues does this PR fix or reference?

https://issues.redhat.com/browse/CRW-5015

How to test this PR?

  1. Deploy che from the PR image: quay.io/eclipse/che-server:pr-609
  2. Create a workspace from a public gitlab-server repository.
  3. Stop the workspace and start it again.

See: workspace starts without any issues.

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

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

Copy link

openshift-ci bot commented Nov 14, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

Copy link

openshift-ci bot commented Nov 14, 2023

@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/v12-azure-no-pat-oauth-flow 379ce02 link true /test v12-azure-no-pat-oauth-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/test-infra repository. I understand the commands that are listed here.

@tolusha
Copy link
Contributor

tolusha commented Nov 14, 2023

What happens if project is private and token is required?
How did it work before?

@vinokurig
Copy link
Contributor Author

/retest

@vinokurig
Copy link
Contributor Author

@tolusha

What happens if project is private and token is required?

If the corresponding oauth config is present, ScmUnauthorizedException will be thrown:

// Do not throw an exception here, proceed the workspace start.
LOG.debug("Failed to update the OAuth token", e);
} catch (ScmUnauthorizedException e) {
throw toApiException(e);
}

and the dashboard will redirect to the authentication page. Otherwise the workspacce will start without the token.

How did it work before?

The UnknownScmProviderException was ignored before this dashboard PR: eclipse-che/che-dashboard#970

@vinokurig vinokurig closed this Nov 14, 2023
@vinokurig
Copy link
Contributor Author

Will rework this in the dashboard side

@vinokurig
Copy link
Contributor Author

Opened the related dashboard PR eclipse-che/che-dashboard#987

@vinokurig vinokurig deleted the CRW-5015 branch November 20, 2023 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants