-
Notifications
You must be signed in to change notification settings - Fork 412
Async Commit/Merge Infrastructure #9732
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
Changes from all commits
9c5b395
738149b
88e7ffa
9ab1c3f
3fd36e6
f2c2799
69135c7
c7dc897
f6195f1
e0cd866
48c87eb
d785292
120db87
69465d6
c904a9f
3cab6c1
15b67b4
0536953
d40500d
adbbf7a
3151b1b
009f716
308f820
8427c7c
0414037
e01f96f
2d2e017
ed14cd8
79d4c4e
6549f89
66e1bbe
f543342
424e6ec
5ac8c9a
386166f
6d512d7
629a5f3
523ab4d
c42a378
e5d3931
30fdc4d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1581,6 +1581,46 @@ components: | |
| - completed | ||
| - update_time | ||
|
|
||
| AsyncTaskStatus: | ||
| type: object | ||
| properties: | ||
| task_id: | ||
| type: string | ||
| description: the id of the async task | ||
| completed: | ||
| type: boolean | ||
| description: true if the task has completed (either successfully or with an error) | ||
| update_time: | ||
| type: string | ||
| format: date-time | ||
| description: last time the task status was updated | ||
| error: | ||
| $ref: "#/components/schemas/Error" | ||
| status_code: | ||
| type: integer | ||
| format: int32 | ||
| description: an http status code that correlates with the underlying error if exists | ||
| required: | ||
| - task_id | ||
| - completed | ||
| - update_time | ||
|
|
||
| MergeAsyncStatus: | ||
| allOf: | ||
| - $ref: "#/components/schemas/AsyncTaskStatus" | ||
| - type: object | ||
| properties: | ||
| result: | ||
| $ref: "#/components/schemas/MergeResult" | ||
|
|
||
| CommitAsyncStatus: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Except result everything here seems to be duplicate of the merge status. Consider consolidating these in a single schema and sharing it among the two |
||
| allOf: | ||
| - $ref: "#/components/schemas/AsyncTaskStatus" | ||
| - type: object | ||
| properties: | ||
| result: | ||
| $ref: "#/components/schemas/Commit" | ||
|
|
||
| PrepareGCUncommittedRequest: | ||
| type: object | ||
| properties: | ||
|
|
@@ -4237,13 +4277,110 @@ paths: | |
| 409: | ||
| $ref: "#/components/responses/Conflict" | ||
| 412: | ||
| description: Precondition Failed (e.g. a pre-commit hook returned a failure) | ||
| $ref: "#/components/responses/PreconditionFailed" | ||
| 429: | ||
| description: too many requests | ||
| default: | ||
| $ref: "#/components/responses/ServerError" | ||
|
|
||
| /repositories/{repository}/branches/{branch}/commits/async: | ||
| parameters: | ||
| - in: path | ||
| name: repository | ||
| required: true | ||
| schema: | ||
| type: string | ||
| - in: path | ||
| name: branch | ||
| required: true | ||
| schema: | ||
| type: string | ||
| post: | ||
| parameters: | ||
| - in: query | ||
| name: source_metarange | ||
| required: false | ||
| description: The source metarange to commit. Branch must not have uncommitted changes. | ||
| schema: | ||
| type: string | ||
| tags: | ||
| - experimental | ||
| operationId: commitAsync | ||
| summary: create commit asynchronously | ||
| requestBody: | ||
| required: true | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: "#/components/schemas/CommitCreation" | ||
| responses: | ||
| 202: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand the reason for 202 but we need to align our spec to return 202 for all async operations this is currently not the case |
||
| description: commit task started | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: "#/components/schemas/Error" | ||
| $ref: "#/components/schemas/TaskCreation" | ||
| 400: | ||
| $ref: "#/components/responses/ValidationError" | ||
| 401: | ||
| $ref: "#/components/responses/Unauthorized" | ||
| 403: | ||
| $ref: "#/components/responses/Forbidden" | ||
| 404: | ||
| $ref: "#/components/responses/NotFound" | ||
| 429: | ||
| description: too many requests | ||
| 501: | ||
| $ref: "#/components/responses/NotImplemented" | ||
| default: | ||
| $ref: "#/components/responses/ServerError" | ||
|
|
||
| /repositories/{repository}/branches/{branch}/commits/async/{id}/status: | ||
| parameters: | ||
| - in: path | ||
| name: repository | ||
| required: true | ||
| schema: | ||
| type: string | ||
| - in: path | ||
| name: branch | ||
| required: true | ||
| schema: | ||
| type: string | ||
| - in: path | ||
| name: id | ||
| required: true | ||
| description: Unique identifier of the commit async task | ||
| schema: | ||
| type: string | ||
| get: | ||
| tags: | ||
| - experimental | ||
| operationId: commitAsyncStatus | ||
| summary: get status of async commit operation | ||
| responses: | ||
| 200: | ||
| description: commit task status | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: "#/components/schemas/CommitAsyncStatus" | ||
| 400: | ||
| $ref: "#/components/responses/ValidationError" | ||
| 401: | ||
| $ref: "#/components/responses/Unauthorized" | ||
| 403: | ||
| $ref: "#/components/responses/Forbidden" | ||
| 404: | ||
| $ref: "#/components/responses/NotFound" | ||
| 409: | ||
| $ref: "#/components/responses/Conflict" | ||
| 412: | ||
| $ref: "#/components/responses/PreconditionFailed" | ||
| 429: | ||
| description: too many requests | ||
| 501: | ||
| $ref: "#/components/responses/NotImplemented" | ||
| default: | ||
| $ref: "#/components/responses/ServerError" | ||
|
|
||
|
|
@@ -4596,6 +4733,113 @@ paths: | |
| default: | ||
| $ref: "#/components/responses/ServerError" | ||
|
|
||
| /repositories/{repository}/refs/{sourceRef}/merge/{destinationBranch}/async: | ||
| parameters: | ||
| - in: path | ||
| name: repository | ||
| required: true | ||
| schema: | ||
| type: string | ||
| - in: path | ||
| name: sourceRef | ||
| required: true | ||
| schema: | ||
| type: string | ||
| description: source ref | ||
| - in: path | ||
| name: destinationBranch | ||
| required: true | ||
| schema: | ||
| type: string | ||
| description: destination branch name | ||
| post: | ||
| tags: | ||
| - experimental | ||
| operationId: mergeIntoBranchAsync | ||
| summary: merge references asynchronously | ||
| requestBody: | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: "#/components/schemas/Merge" | ||
| responses: | ||
| 202: | ||
| description: merge task started | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: "#/components/schemas/TaskCreation" | ||
| 400: | ||
| $ref: "#/components/responses/ValidationError" | ||
| 401: | ||
| $ref: "#/components/responses/Unauthorized" | ||
| 403: | ||
| $ref: "#/components/responses/Forbidden" | ||
| 404: | ||
| $ref: "#/components/responses/NotFound" | ||
| 429: | ||
| description: too many requests | ||
| 501: | ||
| $ref: "#/components/responses/NotImplemented" | ||
| default: | ||
| $ref: "#/components/responses/ServerError" | ||
|
|
||
| /repositories/{repository}/refs/{sourceRef}/merge/{destinationBranch}/async/{id}/status: | ||
| parameters: | ||
| - in: path | ||
| name: repository | ||
| required: true | ||
| schema: | ||
| type: string | ||
| - in: path | ||
| name: sourceRef | ||
| required: true | ||
| schema: | ||
| type: string | ||
| description: source ref | ||
| - in: path | ||
| name: destinationBranch | ||
| required: true | ||
| schema: | ||
| type: string | ||
| description: destination branch name | ||
| - in: path | ||
| name: id | ||
| required: true | ||
| description: Unique identifier of the merge async task | ||
| schema: | ||
| type: string | ||
| get: | ||
| tags: | ||
| - experimental | ||
| operationId: mergeIntoBranchAsyncStatus | ||
| summary: get status of async merge operation | ||
| responses: | ||
| 200: | ||
| description: merge task status | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: "#/components/schemas/MergeAsyncStatus" | ||
| 400: | ||
| $ref: "#/components/responses/ValidationError" | ||
| 401: | ||
| $ref: "#/components/responses/Unauthorized" | ||
| 403: | ||
| $ref: "#/components/responses/Forbidden" | ||
| 404: | ||
| $ref: "#/components/responses/NotFound" | ||
| 409: | ||
| $ref: "#/components/responses/Conflict" | ||
| 412: | ||
| $ref: "#/components/responses/PreconditionFailed" | ||
| 429: | ||
| description: too many requests | ||
| 501: | ||
| $ref: "#/components/responses/NotImplemented" | ||
| default: | ||
| $ref: "#/components/responses/ServerError" | ||
|
|
||
| /repositories/{repository}/branches/{branch}/diff: | ||
| parameters: | ||
| - $ref: "#/components/parameters/PaginationAfter" | ||
|
|
@@ -6701,7 +6945,7 @@ paths: | |
| schema: | ||
| $ref: "#/components/schemas/MergeResult" | ||
| 412: | ||
| description: precondition failed (e.g. a pre-merge hook returned a failure) | ||
| description: precondition failed | ||
| content: | ||
| application/json: | ||
| schema: | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Please consider - not sure what's the point of checking for completion when there's an error. Seems redundant
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.
what do you mean - in order to check if we have an error we first need to see if completed=true, isn't it? Or did I misunderstand you?
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 two are not inter-dependant.
Once you set an error (i.e. it is not nil) it is redundant to check the completed field.
When the client will implement the status loop - it will look for either completed or error. Either one will break the loop.
This is not blocking, I just don't understand this requirement
Uh oh!
There was an error while loading. Please reload this page.
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.
we discussed with Barak yesterday about it