-
Notifications
You must be signed in to change notification settings - Fork 71
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
Use hardcoded string as username for Azure git credentials #529
Conversation
: personalAccessToken.getScmUserName(), | ||
: isNullOrEmpty(personalAccessToken.getScmOrganization()) | ||
? personalAccessToken.getScmUserName() | ||
: personalAccessToken.getScmOrganization(), |
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 haven't tested this yet but this appears to apply to all platforms (e.g. GitHub, etc.). Is this intentional? I'm generally a little confused on what works for all platforms -- e.g. on GitHub my username is amisevsk
and (I guess) my organization name is amisevsk
, even though I'm not an organization?
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.
According to the PAT documentation we use the che.eclipse.org/scm-organization
annotation only for the Azure DevOps. For GitHub and others except Azure, che.eclipse.org/scm-username
is used so the flow is the same.
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.
can we extract this logic to a separate method and add javadoc for that?
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.
done
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 reworked the logic to use hardcoded "username" string for the Azure case. It seems to be safer then an scm-organization
field.
Signed-off-by: Igor Vinokur <ivinokur@redhat.com>
Updated the PR title and description according to the logic 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.
LGTM; haven't had a chance to test directly though.
@vinokurig could you please provide the image for testing? My understanding that with this PR token should work when configured from the user dashboard, right? |
A docker image to test the PR is |
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.
verified against rosa cluster 👍 I think we are good to merge with 3 approvals
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: amisevsk, 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 |
Build 3.8 :: server_3.x/184: Console, Changes, Git Data |
Build 3.8 :: sync-to-downstream_3.x/3747: Console, Changes, Git Data |
What does this PR do?
Since Azure git provider do not support
username
for its user object, useusername
string as username in the git credentials file if the PAT is related to Azure Devops:Screenshot/screencast of this PR
What issues does this PR fix or reference?
fixes eclipse-che/che#22313
How to test this PR?
See: the workspace starts and the project is git cloned successfully.
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
completedReviewers
Reviewers, please comment how you tested the PR when approving it.