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

Fix branch matching based on base name #1352

Merged

Conversation

savitaashture
Copy link
Member

Changes

Fixes : https://issues.redhat.com/browse/SRVKP-3296

After this fix PAC will not run all those PipelineRuns based on branch base name instead will check base name and exact branch name

Before :
Ex: savitaashture/article#362

The test-jira-issue branch have 2 pipelineruns and when we push to me/test/test-jira-issue branch then the push pipeline use to trigger for branch test-jira-issue

After :
Ex: savitaashture/article#369

The test-jira-issue branch have 2 pipelineruns and when we push to m/l/test-jira-issue branch then the push pipeline will not trigger now

Submitter Checklist

  • ♽ Run make test before submitting a PR (ie: with pre-commit, no need to waste CPU cycle on CI. (or even better install pre-commit and do pre-commit install in the root of this repo).
  • ✨ We heavily rely on linters to get our code clean and consistent, please ensure that you have run make lint before submitting a PR. The markdownlint error can get usually fixed by running make fix-markdownlint (make sure it's installed first)
  • 📖 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).

@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Merging #1352 (8b60a3b) into main (1eb8efe) will increase coverage by 0.03%.
Report is 2 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1352      +/-   ##
==========================================
+ Coverage   60.66%   60.70%   +0.03%     
==========================================
  Files         136      136              
  Lines        9913     9909       -4     
==========================================
+ Hits         6014     6015       +1     
+ Misses       3415     3411       -4     
+ Partials      484      483       -1     
Files Changed Coverage Δ
pkg/matcher/annotation_matcher.go 95.20% <ø> (+1.87%) ⬆️

... and 1 file with indirect coverage changes

@piyush-garg
Copy link
Member

lgtm

@@ -28,8 +28,16 @@ const (
func branchMatch(prunBranch, baseBranch string) bool {
// If we have targetBranch in annotation and refs/heads/targetBranch from
// webhook, then allow it.
// Also match for other variables which may exist between refs/heads and targetBranch
// ex: refs/heads/abc/xyz/targetBranch
// In those cases branch matching should fail
if filepath.Base(baseBranch) == filepath.Base(prunBranch) {
Copy link
Member

Choose a reason for hiding this comment

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

do you think we need this if ?

Copy link
Member

Choose a reason for hiding this comment

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

also assume the case if refs/heads is not there in the name, then this will fail

Copy link
Member

Choose a reason for hiding this comment

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

we need to do simple like - if there is ref/head in start, we need to remove that prefix for both names, and then the remaining string would match.

Copy link
Member Author

@savitaashture savitaashture Aug 1, 2023

Choose a reason for hiding this comment

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

do you think we need this if ?

Yes its required for push scenario where no refs/heads specified in targetbranch but the payload coming for push always have appended refs/heads/{branch_name} and filepath.Base returns the last element of the path ignoring the refs/heads so to compare branchname it will be helpful

ex:

	fmt.Println("On Unix:")
	fmt.Println(filepath.Base("/foo/bar/baz.js"))
	fmt.Println(filepath.Base("/foo/bar/baz"))
	fmt.Println(filepath.Base("/foo/bar/baz/"))
	fmt.Println(filepath.Base("dev.txt"))
	fmt.Println(filepath.Base("../todo.txt"))
	fmt.Println(filepath.Base(".."))
	fmt.Println(filepath.Base("."))
	fmt.Println(filepath.Base("/"))
	fmt.Println(filepath.Base(""))

	Output:
	//
	On Unix:
	baz.js
	baz
	baz
	dev.txt
	todo.txt
	..
	.
	/
	.

But the above if condition created issue when there is something inbetween refs/heads and targetbranchname as explained in the comment section as well as PR description

Copy link
Member Author

Choose a reason for hiding this comment

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

we need to do simple like - if there is ref/head in start, we need to remove that prefix for both names, and then the remaining string would match

that we are doing for CEL expression https://github.com/openshift-pipelines/pipelines-as-code/blob/main/pkg/matcher/cel.go#L27-L38

Copy link
Member Author

Choose a reason for hiding this comment

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

we need to do simple like - if there is ref/head in start, we need to remove that prefix for both names, and then the remaining string would match.

actually i had this same thought but filepath.Base does the same operation so kept that condition without changing

Copy link
Member

Choose a reason for hiding this comment

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

i mean the case where prunBranch also contains ref/heads is not handled, also the first if condition is of no use after this implementation

Copy link
Member Author

@savitaashture savitaashture Aug 1, 2023

Choose a reason for hiding this comment

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

Actually it will not fail instead go for checking another condition and succeed

But i rechecked the code and found that removing below if condition and not adding any extra check works

	if filepath.Base(baseBranch) == filepath.Base(prunBranch) {
		return true
	}

Because we have regex comparison in later section to match the branch name

@piyush-garg
Copy link
Member

lgtm

@piyush-garg piyush-garg merged commit eb26ff0 into openshift-pipelines:main Aug 1, 2023
4 checks passed
osp-pac pushed a commit that referenced this pull request Aug 1, 2023
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.

2 participants