Design to keep PullRequest PipelineResource in sync with reality #1286
Comments
|
One way to work around this (reduce the risk) would be to use this with a Notification type (#49 ) |
|
For our beta release, maybe more realistic to view PR outputs as additive only. (would imply not being able to remove?) |
|
might be useful: https://developer.github.com/v4/mutation/ |
|
I don't think notification types would solve this problem. The race condition exists even in that case, since a new comment could come in from when the PR is initialized on disk. I think what we want here is a comment manifest. This would let us detect whether or not a comment was explicitly deleted, or just one we don't know about yet. e.g. When we download the pull request resource, also create a file called When we upload the PR, if we see that 2.json is missing from the local filesystem, we know this was a deletion from the Task because it was originally listed in the manifest. If we see that 3.json is missing from the local filesystem, we shouldn't make any modifications to it because the task was not aware of it when the PR resource was initialized since it was not in the manifest. |
Introduces `.MANIFEST` files to comment and label directories to keep track of sub-resources when the PullRequest resource is initialized. This allows us to keep track of what resources the user explicitly deleted, and not accidentally overwrite new comments/lables that were introduced during execution. Minor changes included: * Changed fake GitHub server to make copies of PullRequests to avoid pointer collision across tests. * Changed uploadLabels to Add/Delete labels explicitly instead of replacing all. Change manifest path to .MANIFEST to avoid collisions. Fixes tektoncd#1286
Introduces `.MANIFEST` files to comment and label directories to keep track of sub-resources when the PullRequest resource is initialized. This allows us to keep track of what resources the user explicitly deleted, and not accidentally overwrite new comments/lables that were introduced during execution. Minor changes included: * Changed fake GitHub server to make copies of PullRequests to avoid pointer collision across tests. * Changed uploadLabels to Add/Delete labels explicitly instead of replacing all. Change manifest path to .MANIFEST to avoid collisions. Fixes tektoncd#1286
Introduces `.MANIFEST` files to comment and label directories to keep track of sub-resources when the PullRequest resource is initialized. This allows us to keep track of what resources the user explicitly deleted, and not accidentally overwrite new comments/lables that were introduced during execution. Minor changes included: * Changed fake GitHub server to make copies of PullRequests to avoid pointer collision across tests. * Changed uploadLabels to Add/Delete labels explicitly instead of replacing all. Change manifest path to .MANIFEST to avoid collisions. Fixes tektoncd#1286
Introduces `.MANIFEST` files to comment and label directories to keep track of sub-resources when the PullRequest resource is initialized. This allows us to keep track of what resources the user explicitly deleted, and not accidentally overwrite new comments/lables that were introduced during execution. Minor changes included: * Changed fake GitHub server to make copies of PullRequests to avoid pointer collision across tests. * Changed uploadLabels to Add/Delete labels explicitly instead of replacing all. Change manifest path to .MANIFEST to avoid collisions. Fixes tektoncd#1286
Introduces `.MANIFEST` files to comment and label directories to keep track of sub-resources when the PullRequest resource is initialized. This allows us to keep track of what resources the user explicitly deleted, and not accidentally overwrite new comments/lables that were introduced during execution. Minor changes included: * Changed fake GitHub server to make copies of PullRequests to avoid pointer collision across tests. * Changed uploadLabels to Add/Delete labels explicitly instead of replacing all. Change manifest path to .MANIFEST to avoid collisions. Fixes tektoncd#1286
Introduces `.MANIFEST` files to comment and label directories to keep track of sub-resources when the PullRequest resource is initialized. This allows us to keep track of what resources the user explicitly deleted, and not accidentally overwrite new comments/lables that were introduced during execution. Minor changes included: * Changed fake GitHub server to make copies of PullRequests to avoid pointer collision across tests. * Changed uploadLabels to Add/Delete labels explicitly instead of replacing all. Change manifest path to .MANIFEST to avoid collisions. Fixes #1286
Expected Behavior
If a Task uses a PullRequest Pipeline resource, as the Task progresses, if comments are added (or other changes represented in the filesystem) to the Pull Request, it should be possible:
Actual Behavior
When a Task is executing, the representation on the file system of the pull request is not updated. Even worse, the side effect is that when the PullRequestResource is used as an output, the data is treated as the source of truth and so any changes made between when the Task started and when it completed can be lost (e.g. comments made in the meantime will be deleted!!!)
Steps to Reproduce the Problem
a. Task could sleep for a minute or something (to give you time for step 3)
b. Task could also add a comment to the output (i.e. write to /workspace//comments/)
Additional Info
We should at least make it so changes are additive, but also we could be ambitious and do something really cool e.g. create our own fs? (@dlorenc had some cool ideas here)
The text was updated successfully, but these errors were encountered: