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 filter pipelinerun by event title #769

Conversation

chmouel
Copy link
Member

@chmouel chmouel commented Jul 25, 2022

Let filter events by title, using CEL

add event_title which support push and pull request

Fixes #768

need to go after #751

Changes

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 changed the title Allow matching event to pipeline by modified files for GitHub pull re… Allow filter pipelinerun by event title Jul 25, 2022
@chmouel chmouel force-pushed the issue-768-filter-pull-request-events-by-title-using-advanced-ex branch 2 times, most recently from 07ac744 to a90e2dc Compare July 25, 2022 12:19
@codecov-commenter
Copy link

codecov-commenter commented Jul 25, 2022

Codecov Report

Merging #769 (0c28499) into main (da4638c) will increase coverage by 0.63%.
The diff coverage is n/a.

❗ Current head 0c28499 differs from pull request most recent head 12c21ee. Consider uploading reports for the commit 12c21ee to get more accurate results

@@            Coverage Diff             @@
##             main     #769      +/-   ##
==========================================
+ Coverage   65.25%   65.88%   +0.63%     
==========================================
  Files          75       75              
  Lines        4533     4494      -39     
==========================================
+ Hits         2958     2961       +3     
+ Misses       1295     1252      -43     
- Partials      280      281       +1     
Impacted Files Coverage Δ
pkg/matcher/cel.go 68.00% <0.00%> (-13.14%) ⬇️
pkg/matcher/annotation_matcher.go 91.22% <0.00%> (-2.64%) ⬇️
pkg/params/clients/clients.go 10.84% <0.00%> (-0.27%) ⬇️
pkg/matcher/annotation_tasks_install.go 84.12% <0.00%> (-0.25%) ⬇️
pkg/templates/templating.go 100.00% <0.00%> (ø)
pkg/provider/bitbucketcloud/bitbucket.go 77.77% <0.00%> (+1.00%) ⬆️
pkg/provider/bitbucketserver/bitbucketserver.go 84.72% <0.00%> (+1.16%) ⬆️
pkg/provider/gitlab/gitlab.go 82.30% <0.00%> (+2.62%) ⬆️
pkg/provider/github/github.go 68.39% <0.00%> (+3.04%) ⬆️
pkg/sort/repository_status.go 80.00% <0.00%> (+5.00%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da4638c...12c21ee. Read the comment docs.

Comment on lines 82 to 97
func (t celPac) pathChanged(vals ref.Val) ref.Val {
var match types.Bool
fileList, err := t.vcx.GetFiles(t.ctx, t.event)
if err != nil {
return types.Bool(false)
}
for i := range fileList {
if v, ok := vals.Value().(string); ok {
g := glob.MustCompile(v)
if g.Match(fileList[i]) {
return types.Bool(true)
}
}
match = types.Bool(false)
}

Copy link
Member

Choose a reason for hiding this comment

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

@chmouel I think these changes are not required here

I believe you added changes on existing code copy

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah this PR is a followup to #751

@chmouel chmouel force-pushed the issue-768-filter-pull-request-events-by-title-using-advanced-ex branch from d0527b5 to 55e8712 Compare July 26, 2022 12:21
Copy link
Member

@savitaashture savitaashture left a comment

Choose a reason for hiding this comment

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

/lgtm

👍 🎉

@savitaashture
Copy link
Member

/hold

This supports push and pull_request (pull request title and sha event)

Closes openshift-pipelines#768

Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
some spagetthi code to make test annotation_matcher on github and gitlab
@chmouel chmouel force-pushed the issue-768-filter-pull-request-events-by-title-using-advanced-ex branch from 12c21ee to 78488bd Compare July 26, 2022 15:18
@chmouel
Copy link
Member Author

chmouel commented Jul 26, 2022

e2e tests should work now, aside of the annoying token expiration limit

@chmouel chmouel merged commit 485b76d into openshift-pipelines:main Jul 26, 2022
@chmouel chmouel deleted the issue-768-filter-pull-request-events-by-title-using-advanced-ex branch July 26, 2022 15:30
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.

Filter pull-request events by title, using Advanced expressions via CEL
3 participants