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

Allow multiple pipelinerun for the same event #589

Merged
merged 7 commits into from Apr 20, 2022

Conversation

chmouel
Copy link
Member

@chmouel chmouel commented Apr 14, 2022

Changes

Allow multiple pipelinerun on the same event

Fixes #528

A bunch of of refactoring to break out .Run which got moved to its own.

We have a sync.Map of checkRunIds to track the checkRuns to which pipelineRun, we use github api "external_id" field for that .

We run a goroutine in a workgroups waiting for all the pr to run This wait that all of them finishes before exiting to the adapter goroutine.
We retries if we can't update the RepoStatus until we are able to do it.

TODO (before merge):

  • Doc
  • E2E tests on multiple PR matching
  • Test that webhook and other provider works (should be creating a new comment)

TODO (post):

  • Increase unittesting scenarios
  • Add /test prname support

Screenshot:

image
image

Submitter Checklist

  • β™½ Run make test lint before submitting a PR (ie: with pre-commit, no need to waste CPU cycle on CI
  • πŸ“– If you are adding a user facing feature or make a change of the behavior, please verify that you have documented it
  • πŸ§ͺ 100% coverage is not a target but most of the time we would rather have a unit test if you make a code change.
  • 🎁 If that's something that is possible to do please ensure to check if we can add a e2e test.
  • πŸ”Ž If there is a flakiness in the CI tests then don't necessary ignore it, better get the flakyness fixed before merging or if that's not possible there is a good reason to bypass it. (token rate limitation may be a good reason to skip).

@chmouel chmouel force-pushed the multiplex branch 2 times, most recently from cad521b to dfc6238 Compare April 20, 2022 09:56
}

if annotationRepo.Spec.URL != "" {
repo = annotationRepo
// TODO: We need to figure out secretCreation, it's buggy normally without multiplexing ie:bug #543
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if we create one secret for each pipeline?
anyway the secret is short lived till the life of pr

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

till we find a way to enable this again what would be side effect of this change?
multiple pipeline from private repo would fail right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let us work on this in a following PR, i may have a nice solution,

@chmouel
Copy link
Member Author

chmouel commented Apr 20, 2022

the e2e test error are legit, just finishing to fix it.

@@ -94,7 +94,7 @@ spec:
},
},
{
name: "test-annotations-inside-repo",
name: "test-annotations-inside-Repo",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this a find and replace repo -> Repo
change ? πŸ˜„

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just un Alt-c too much :D

Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
@sm43
Copy link
Contributor

sm43 commented Apr 20, 2022

also how this is suppose to work with webhooks ?
one message for each pipelinerun outcome ?

@chmouel
Copy link
Member Author

chmouel commented Apr 20, 2022

also how this is suppose to work with webhooks ?
one message for each pipelinerun outcome ?

the same as before ?

what do you mean by one message?

(there was a bug in e2e with webhooks just before which i just fixed)

we now gets all the matched condition from the matcher in the same struct

we did a bunch of refactoring to break out .Run which got moved to its own

Make event able to process multiple pipelineruns

We run a goroutine in a workgroups which we wait for. This will be run in the
main goroutine which started from the sinker.

We disable secret auto creation for now when running multiples, it has many
issues we need to fix regardless.

We retries if we can't update the RepoStatus until we are able to do it.

Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
@sm43
Copy link
Contributor

sm43 commented Apr 20, 2022

also how this is suppose to work with webhooks ?
one message for each pipelinerun outcome ?

the same as before ?

what do you mean by one message?

(there was a bug in e2e with webhooks just before which i just fixed)

while using webhook we post a comment with breakdown
so for multiple pipelinerun
each pr will have a comment I think?
I was thinking there would be too many comments but thats ok I guesss

@chmouel
Copy link
Member Author

chmouel commented Apr 20, 2022

yeah correct, or i believe it will, i have just enabled multiple prs e2e test for gitlab so will see how it goes πŸ™ƒ

@sm43
Copy link
Contributor

sm43 commented Apr 20, 2022

image
bitbucket πŸ€”

@chmouel
Copy link
Member Author

chmouel commented Apr 20, 2022

Gitlab is fine now and looks like this :

image

I am not sure about bitbucket yet πŸ˜… gitlab api is a bit awkward since you can't upload multiple files at the same time, will see if bitbucket is the same..

Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
@chmouel
Copy link
Member Author

chmouel commented Apr 20, 2022

Bitbucket is okay now :

image

@sm43
Copy link
Contributor

sm43 commented Apr 20, 2022

may be we should add which pipeline name in comment ?
to know to which pipeline the result belong to
(may be we can improve as we go)

@chmouel
Copy link
Member Author

chmouel commented Apr 20, 2022

@sm43 yeah already created a issue for it πŸ˜† #604

@chmouel chmouel added this to the 0.7.0 milestone Apr 20, 2022
@chmouel chmouel changed the title Allow multiple pipelienrun for the same event Allow multiple pipelinerun for the same event Apr 20, 2022
Co-authored-by: Shivam Mukhade <smukhade@redhat.com>
@sm43 sm43 merged commit 0d63e62 into openshift-pipelines:main Apr 20, 2022
@chmouel chmouel deleted the multiplex branch April 20, 2022 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Allow running multiple matching PipelineRuns
2 participants