-
Notifications
You must be signed in to change notification settings - Fork 139
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
Make it possible to use multiple tokens. #186
base: master
Are you sure you want to change the base?
Conversation
This commit is based on the work of @valentinschabschneider and updated to the latest master branch. deckweiss@9653c35#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80 Fixes piceaTech#104.
This looks good to me and very useful if one is migrating a repository which is in use and has multiple collaborators. @spruce |
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 like the changes. Except some leftover comments. If those will be removed, I'm happy.
settings.github.token_owner) || | ||
author.username === settings.github.token_owner) | ||
this.githubApis[settings.usermap[author.username as string]]) || | ||
// settings.usermap[author.username as string] === |
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 you remove the comments. So we don't clutter the codebase
settings.github.token_owner) || | ||
note.author.username === settings.github.token_owner; | ||
let userIsPoster = this.userTokenAvailable(note.author); | ||
// (settings.usermap && |
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.
Remove these please, too.
@@ -917,7 +929,8 @@ export class GithubHelper { | |||
let bodyConverted = await this.convertIssuesAndComments( | |||
mergeStr + mergeRequest.description, | |||
mergeRequest, | |||
!this.userIsCreator(mergeRequest.author) || !settings.useIssueImportAPI | |||
!this.userTokenAvailable(mergeRequest.author) // || | |||
//!settings.useIssueImportAPI |
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.
This one as well.
Thank you for the nice PR! We try to use this PR for migrating our Issues from GitLab to GitHub. Using multiple tokens to connect user accounts seems to work as expected without using the |
This PR is based on the work of @valentinschabschneider and updated to the latest master branch:
deckweiss@9653c35#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80
Fixes #104.