-
Notifications
You must be signed in to change notification settings - Fork 417
OSS WebUI Support for Async Commit/Merge WebUI implementation #9783
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
base: master
Are you sure you want to change the base?
Conversation
itaigilo
left a comment
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.
Nice and clean plugin implementation!
I think that the plugin should be split (see comment in PluginCommitMergeStrategy) though.
| await refsAPI.merge(repo.id, source, dest, strategy, message, metadata); | ||
| await pluginManager.commitMergeStrategy.merge(repo.id, source, dest, { | ||
| message: message || undefined, | ||
| metadata: Object.keys(metadata).length > 0 ? metadata : undefined, |
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 think it's much better to handle this in the implementation itself (and avoid checking this here).
Same goes for message.
| force: false, | ||
| allow_empty: false, | ||
| squash_merge: false |
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.
Since these are always false, I think they should better be removed.
| reference: string; | ||
| } | ||
|
|
||
| export interface PluginCommitMergeStrategy { |
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.
In the OSS world, commit and merge are not related -
Other than the fact that they both have an async option.
Hence, let's split this into a commit plugin and a merge plugin -
It will require very little extra code.
In addition, the terminology isn't right:
The plugin doesn't provide the "strategy" (for some other component to use), but an actual implementation.
There might be better naming suggestions than the following, but I suggest:
export interface PluginCommitAction {
run(): Promise<CommitResult>;
}
export interface PluginMergeAction {
run(): Promise<MergeResult>;
}
itaigilo
left a comment
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.
Looking good, nicely done!
Only Esti fixes, and you should be good to go.
|
@itaigilo |
Talked on Slack, all good on my side. |
Closes #9781
Change Description
This PR adds the plugin to implement the WebUI client logic for using the async commit and merge functionality in Enterprise.
Testing
Added a Playwright test.
The Enterprise PR that implements the functionality is here.