-
Notifications
You must be signed in to change notification settings - Fork 336
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
feat: Enable connecting to different GitLab integration providers #10025
Conversation
WalkthroughThe recent updates enhance the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
d6a2066
to
eef4028
Compare
@ackernaut could you check the loom if it's good enough? |
@Dschoordsch I watched the Loom video. I think this is a great start. Nice work. A few small things:
|
Adding a simple edit/delete menu required some big refactoring.
b700f17
to
7cdee22
Compare
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
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.
Hey @Dschoordsch, how can I test the GitLab server integration? Where can I get the server base url etc?
When I go to the team integrations tab, I see this at the moment:
Configure organization-level integrations that can be used by teams across your | ||
organization. | ||
<br /> | ||
See the team integration tab for team-level connections. |
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.
+1 adding a hyperlink to the team integrations would be helpful here
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.
Yes, but I wouldn't know to which team I should link
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
I fixed the row when no provider is available (normally we would have the global one already). |
{isDesktop ? ( | ||
<> | ||
<div className='flex items-center pr-6'> | ||
<DoneIcon className='h-[18px] w-[18px] text-lg text-success-light' /> |
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.
+1 I think it's best to use the default tailwind heights where possible rather than [6px] [18px] etc
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.
Yes, I agree. I just did not want to modify the existing layout in this PR. It's too big already.
}, | ||
'success-light': '#2db553', | ||
starter: '#F2E1F7', | ||
team: '#CBECF0', |
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.
+1 I can see success-light
being used in this PR, but not starter, team or enterprise?
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 added these just for completeness, so the old PALETTE and the tailwind colours match.
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.
Tested and it worked!
</div> | ||
<div className='flex items-center p-2'> | ||
<div className='w-36'>Redirect URI</div> | ||
<CopyShortLink url={redirectUri} title='Redirect URL' tooltip='Copy Redirect URL' /> |
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.
+1 this looked like a hyperlink to me. I tried to copy the text by highlighting it, but I wasn't able to highlight it. I then accidentally clicked on it and saw the "Copy Redirect URL" tooltip.
I think it'd be more user friendly if it had a copy button next to the URL, or if it looked more similar to the invite link:
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.
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.
Love it!
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
Description
Fixes #10024
[Please include a summary of the changes and the related issue]
Demo
https://www.loom.com/share/a5a3aa15f716401cb2e3d5458fcda8a0?sid=fa83daa9-4e18-4c98-bf83-ec348a778ea1
Testing scenarios
Test UI with existing team webhook integrations (Mattermost and MS Teams)
test adding, removing, updating and see the changes reflected in UI
Test UI with 1 or more dummy GitLab integration providers
add, remove and edit the providers
test as org admin and without rights
Add a real GitLab integration provider and connect to it via the team integrations page
Final checklist
Summary by CodeRabbit
New Features
GitLabProviderRow
component to display multiple GitLab instances and their connection statuses.ProviderCard
layout for clearer presentation of provider information and actions.Bug Fixes
tenantId
field in the GraphQL API, allowing it to be optional for greater flexibility in submissions.