-
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
[DO NOT MERGE] Delete personal access tokens without provider name annotation #685
Conversation
[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 |
74be61d
to
a613555
Compare
/retest |
19340ea
to
1b993bc
Compare
/retest |
1 similar comment
/retest |
@vinokurig, So, I think the PR can be merged. |
/retest |
4 similar comments
/retest |
/retest |
/retest |
/retest |
For some reason that is still unclear to me, the |
See the screenshot: |
The pull request already changes the token secret content. I add the provider name label to the yaml template. However the test fails because it fetches the template from main, not from the pr branch. |
@vinokurig If to change the values in the annotations of
I mean, seems the |
This can be reproduced on dogfooding instance, so this is not related to the PR. Please create an issue for that. Note that the token must be added by the |
fixed |
The
I guess, it's related to an infrastructure problem. Doing to rerun. |
/retest |
I deployed I tried two variants of
|
Not sure, could you reproduce the case on dogfooding instance? |
I also can't reproduce it on |
|
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.
@vinokurig would it affect PAT that were configured before 7.80.0 ? my understanding that they would be brutally deleted after new version and if a user leveraged PAT flow they would not be able to work until configuring a new PAT from scratch, right?
yes, looks like we need a decision here |
Not a priority issue. |
What does this PR do?
Check a personal access token to have the
che.eclipse.org/scm-provider-name
annotation instead of the deprecatedche.eclipse.org/scm-personal-access-token-name
. All token secrets that were created before the 7.84 version, will be removed. A new secret with the new annotation will be created on workspace create/start.Screenshot/screencast of this PR
What issues does this PR fix or reference?
https://issues.redhat.com/browse/CRW-6301
How to test this PR?
quay.io/eclipse/che-server:pr-685
.personal-access-token-...
secret.che.eclipse.org/scm-provider-name
annotation.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.