-
-
Notifications
You must be signed in to change notification settings - Fork 195
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
2822 gitlab sync is not allowing users to push tokens #2854
2822 gitlab sync is not allowing users to push tokens #2854
Conversation
|
@@ -101,7 +101,7 @@ export function useGitLab() { | |||
themes, | |||
metadata: {}, | |||
}; | |||
} catch (e) { | |||
} catch (e: any) { |
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.
} catch (e: any) { | |
} catch (e: Error) { |
This would save checking e instanceof Error
below, no?
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.
No. The error type Gitbeaker
module returns is not Error.
So it need to be flexible.
break; | ||
} | ||
default: | ||
throw new Error('Not implemented'); | ||
} | ||
if (pushResult.status && String(pushResult.status).includes('failure')) { |
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.
Why does the status need to be cast to a string
? Is it not always status: 'failure'
or status: 'success'
? Then it should work.
if (pushResult.status && String(pushResult.status).includes('failure')) { | |
if (pushResult.status && pushResult.status === 'failure')) { |
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.
Updated.
break; | ||
} | ||
default: | ||
throw new Error('Not implemented'); | ||
} | ||
if (pushResult.status && String(pushResult.status).includes('failure')) { | ||
notifyToUI(String(pushResult.errorMessage), { error: true }); |
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.
Similar question, in what cases are the errorMessage
s not a string? We should be handling these to being strings before throwing.
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.
Updated
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.
These messages should be translated and offer an extensive list. Created a ticket: #2856
Why does this PR exist?
Current plugin has no feature to let the users know if there is an error during pushing tokens to remote provider.
Closes #2822
What does this pull request do?
Added a feature, so show message to users if there is an error during pushing tokens to remote providers.
Testing this change
Additional Notes (if any)
20240614_032237.mp4