Skip to content
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

Broken logic for pull request statuses in PR resource #2188

Closed
afrittoli opened this issue Mar 8, 2020 · 2 comments · Fixed by #2194
Closed

Broken logic for pull request statuses in PR resource #2188

afrittoli opened this issue Mar 8, 2020 · 2 comments · Fixed by #2194
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@afrittoli
Copy link
Member

Expected Behavior

When a check is updated in the PR pipeline resource, the corresponding check should be updated on github side.

Actual Behavior

The update may happen or not depending on the case.

Additional Info

The code in

csMap := map[string]scm.State{}
for _, s := range cs {
csMap[s.Label] = s.State
}
makes the assumption that the label, or context in case of GitHiub, is a unique key to identify a "check". That is not the case for GitHub - the API model maintains a list of checks, which may have the same context, but different state and id.

The reason why this issue happens is that in some cases the code in the pullrequest/api packages makes a map of the checks based on the label, so the stated stored in the map may be that of any of the status available.

Even if it was considered responsibility of the CI system to manage these multiple states, the pull request resource does not make that possible by hiding the check id away.

When a process in the CI system sends an update to a status, it should get back the ID from github. That ID can then be used by the CI system to update the correct status once the CI job is complete.

Unfortunately the GitHub API docs don't really explain more in which cases a new status is created even if one exists already with the same name. It might be that that happens every time an update is sent without the ID. Or it may be that the API allows for multiple statues against the same job to allow for parallel checks. I would say the former is more likely, but I struggle to understand how we could end up then with 7 different IDs for the same context like in the example from the gist.

@vdemeester
Copy link
Member

/kind bug

@tekton-robot tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Mar 9, 2020
afrittoli added a commit to afrittoli/pipeline that referenced this issue Mar 9, 2020
The GitHub API keeps old versions of a status for historical
reasons. The statuses are returned in reverse chronological
order. Let's ignore everything but the most recent status, both
when storing the resource to disk, as well as when checking for
the need to update before the upload.

Fixes tektoncd#2188
afrittoli added a commit to afrittoli/pipeline that referenced this issue Mar 10, 2020
The GitHub API keeps old versions of a status for historical
reasons. The statuses are returned in reverse chronological
order. Let's ignore everything but the most recent status, both
when storing the resource to disk, as well as when checking for
the need to update before the upload.

Fixes tektoncd#2188
afrittoli added a commit to afrittoli/pipeline that referenced this issue Mar 10, 2020
The GitHub API keeps old versions of a status for historical
reasons. The statuses are returned in reverse chronological
order. Let's ignore everything but the most recent status, both
when storing the resource to disk, as well as when checking for
the need to update before the upload.

Fixes tektoncd#2188
afrittoli added a commit to afrittoli/pipeline that referenced this issue Mar 10, 2020
The GitHub API keeps old versions of a status for historical
reasons. The statuses are returned in reverse chronological
order. Let's ignore everything but the most recent status, both
when storing the resource to disk, as well as when checking for
the need to update before the upload.

Fixes tektoncd#2188
afrittoli added a commit to afrittoli/pipeline that referenced this issue Mar 10, 2020
The GitHub API keeps old versions of a status for historical
reasons. The statuses are returned in reverse chronological
order. This patch pulls-in a version of go-scm that handles
this behaviour and only returns the latest status.

Fixes tektoncd#2188
afrittoli added a commit to afrittoli/pipeline that referenced this issue Mar 10, 2020
The GitHub API keeps old versions of a status for historical
reasons. The statuses are returned in reverse chronological
order. This patch pulls-in a version of go-scm that handles
this behaviour and only returns the latest status.

Fixes tektoncd#2188

Signed-off-by: Andrea Frittoli <andrea.frittoli@uk.ibm.com>
afrittoli added a commit to afrittoli/pipeline that referenced this issue Mar 10, 2020
The GitHub API keeps old versions of a status for historical
reasons. The statuses are returned in reverse chronological
order. This patch pulls-in a version of go-scm that handles
this behaviour and only returns the latest status.

Fixes tektoncd#2188

Signed-off-by: Andrea Frittoli <andrea.frittoli@uk.ibm.com>
afrittoli added a commit to afrittoli/pipeline that referenced this issue Mar 10, 2020
The GitHub API keeps old versions of a status for historical
reasons. The statuses are returned in reverse chronological
order. This patch pulls-in a version of go-scm that handles
this behaviour and only returns the latest status.

Fixes tektoncd#2188

Signed-off-by: Andrea Frittoli <andrea.frittoli@uk.ibm.com>
tekton-robot pushed a commit that referenced this issue Mar 10, 2020
The GitHub API keeps old versions of a status for historical
reasons. The statuses are returned in reverse chronological
order. This patch pulls-in a version of go-scm that handles
this behaviour and only returns the latest status.

Fixes #2188

Signed-off-by: Andrea Frittoli <andrea.frittoli@uk.ibm.com>
tekton-robot pushed a commit that referenced this issue Mar 10, 2020
The GitHub API keeps old versions of a status for historical
reasons. The statuses are returned in reverse chronological
order. This patch pulls-in a version of go-scm that handles
this behaviour and only returns the latest status.

Fixes #2188

Signed-off-by: Andrea Frittoli <andrea.frittoli@uk.ibm.com>
(cherry picked from commit a5794f2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants