Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add Azure DevOps Server support #754

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add Azure DevOps Server support #754

wants to merge 1 commit into from

Conversation

vinokurig
Copy link
Contributor

@vinokurig vinokurig commented Jan 20, 2025

What does this PR do?

Depends on eclipse-che/che-dashboard#1313

Screenshot/screencast of this PR

What issues does this PR fix or reference?

fixes eclipse-che/che#23306

How to test this PR?

  1. Deploy che with the pull request image: quay.io/eclipse/che-server:pr-754 and quay.io/eclipse/che-dashboard:pr-1313
  2. In the Dev azure Server instance create a personal access token with full access.
  3. Create a personal access token, use the Organization input to enter the Collection name.
  4. Start a workspace from an azure devops server repository with a devfile.

See: workspace starts with the devfile resolve.

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.

Copy link

openshift-ci bot commented Jan 20, 2025

[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

@vinokurig vinokurig marked this pull request as draft January 20, 2025 14:40
@vinokurig vinokurig force-pushed the che-23306 branch 2 times, most recently from 259113e to 4c865ce Compare January 21, 2025 12:58
@vinokurig vinokurig marked this pull request as ready for review January 21, 2025 13:35
@vinokurig
Copy link
Contributor Author

/retest

1 similar comment
@vinokurig
Copy link
Contributor Author

/retest

}

protected AzureDevOpsUrl withDevfileFilenames(List<String> devfileFilenames) {
this.devfileFilenames.addAll(devfileFilenames);
return this;
}

public AzureDevOpsUrl withServerUrl(String serverUrl) {
this.serverUrl = serverUrl;
Copy link
Contributor

Choose a reason for hiding this comment

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

When use set serverUrl, then hostname is null, which is used later in getRepositoryLocation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The hostName does not rely on serverUrl, we set it independently:

return new AzureDevOpsUrl()
.withHostName(
url.startsWith("git@ssh.") ? azureDevOpsScmApiEndpointHost : URI.create(url).getHost())

@@ -172,8 +172,19 @@ public Optional<Boolean> isValid(PersonalAccessToken personalAccessToken) {
public Optional<Pair<Boolean, String>> isValid(PersonalAccessTokenParams params)
throws ScmCommunicationException {
if (!isValidScmServerUrl(params.getScmProviderUrl())) {
LOG.debug("not a valid url {} for current fetcher ", params.getScmProviderUrl());
return Optional.empty();
if (OAUTH_PROVIDER_NAME.equals(params.getScmProviderName())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we refactor this fuction?
For instance, move if (OAUTH_PROVIDER_NAME.equals(params.getScmProviderName())) { before if (!isValidScmServerUrl(params.getScmProviderUrl())) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to check if the token belongs to Azure DevOps SAAS or Server first. The isValidScmServerUrl function name is a bit misleading, so I renamed it.

azureDevOpsApiClient.getUserWithPAT(
personalAccessToken.getToken(), personalAccessToken.getScmOrganization());
return new GitUserData(user.getDisplayName(), user.getEmailAddress());
if (personalAccessToken.getScmProviderUrl().equals("https://dev.azure.com")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have AzureDevOps class for contants

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -112,7 +112,8 @@ private boolean isApiRequestRelevant(String repositoryUrl) {
// belongs to Gitlab.
gitlabApiClient.getOAuthTokenInfo("");
} catch (ScmUnauthorizedException e) {
return true;
// the error message is a JSON if it is a response from Gitlab.
return e.getMessage().startsWith("{");
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this changes in context of azure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we need to specify the unauthorized exception as Azure also returns an unauthorized exception in this case.

Copy link
Member

Choose a reason for hiding this comment

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

what is the response from Azure in this case?


private Optional<Matcher> getPatternMatcherByUrl(String url) {
String host = URI.create(url).getHost();
Matcher matcher = compile(format(azureDevOpsPatternTemplate, host)).matcher(url);

Check failure

Code scanning / CodeQL

Regular expression injection High

This regular expression is constructed from a
user-provided value
.
This regular expression is constructed from a
user-provided value
.
This regular expression is constructed from a
user-provided value
.
if (matcher.matches()) {
return Optional.of(matcher);
} else {
matcher = compile(format(azureSSHDevOpsPatternTemplate, host)).matcher(url);

Check failure

Code scanning / CodeQL

Regular expression injection High

This regular expression is constructed from a
user-provided value
.
This regular expression is constructed from a
user-provided value
.
This regular expression is constructed from a
user-provided value
.
if (matcher.matches()) {
return Optional.of(matcher);
} else {
matcher = compile(format(azureSSHDevOpsServerPatternTemplate, host)).matcher(url);

Check failure

Code scanning / CodeQL

Regular expression injection High

This regular expression is constructed from a
user-provided value
.
This regular expression is constructed from a
user-provided value
.
This regular expression is constructed from a
user-provided value
.
@vinokurig vinokurig closed this Feb 5, 2025
@vinokurig vinokurig deleted the che-23306 branch February 5, 2025 14:01
@vinokurig vinokurig restored the che-23306 branch February 5, 2025 14:01
@vinokurig vinokurig reopened this Feb 5, 2025
@vinokurig vinokurig force-pushed the che-23306 branch 2 times, most recently from 80c8b96 to 12721bc Compare February 6, 2025 12:00
@vinokurig vinokurig marked this pull request as draft February 7, 2025 08:43
@vinokurig vinokurig force-pushed the che-23306 branch 4 times, most recently from cfaf300 to 95d556a Compare February 10, 2025 08:40
Copy link
Member

@ibuziuk ibuziuk left a comment

Choose a reason for hiding this comment

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

Based on today's discussion it was decided to keep the initial implementation for supporting Azure DevOps TFS Server as simple as possible:

@vinokurig
Copy link
Contributor Author

/retest

Copy link

openshift-ci bot commented Feb 19, 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 0372ac8 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.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Microsoft Azure DevOps Server (TFS)
3 participants