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

Comparator Jenkins intergration #8

Merged
merged 12 commits into from
Oct 26, 2023

Conversation

patrikcerbak
Copy link
Collaborator

@patrikcerbak patrikcerbak commented Oct 19, 2023

Comparator Jenkins intergration

Added a Comparator links section into the Jenkins global config in which you can match a job by regex and then add multiple links to Comparator tool. Each link has a label, spliterator (a regex used to split the job name into multiple "elements") and a field to enter comparator arguments.

When you enter a --regex argument, the given value is taken and evaluated, there are some "wildcards" you can use in this query:

  • %{X} (you can use any number in place of X) - this evaluates to the Xth element of the split job name
  • %{N-X} (you can use any number in place of X) - this evaluates to the Xth LAST element of the split job name
  • %{S} or %{SPLIT} - this evaluates to the spliterator expression

The links are added to the java-reports page

@patrikcerbak patrikcerbak requested a review from a team as a code owner October 19, 2023 15:43
@judovana
Copy link
Contributor

That commit is surprsingly short... Kudos!
At least one new method deserves unittest. Actually more of older, existing (and some of them now touched) deserves unittests...

return url.toString()
.replace(" ", "+")
.replace("#", "%23")
.replace(".", "%2E");
Copy link
Contributor

Choose a reason for hiding this comment

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

There are methods which will escape the compelte url with al trics and treats in it.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are methods which will escape the compelte url with al trics and treats in it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, it is now using URLEncoder.encode() instead of this.

url.append(parseToRegex(ltc.getSpliterator(), arg.split(" ")[1])); // split the arg my space and give it the second part
} else {
url.append(arg);
url.append("+");
Copy link
Contributor

Choose a reason for hiding this comment

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

I would recomend to append by " " and then escape whole url

Copy link
Contributor

Choose a reason for hiding this comment

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

I would recomend to append by " " and then escape whole url

for (String arg : ltc.getComparatorArguments().split("\n")) {
if (arg.matches("^-+regex.*")) {
url.append("--regex+");
url.append(parseToRegex(ltc.getSpliterator(), arg.split(" ")[1])); // split the arg my space and give it the second part
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks wrong. I would do it for any arg.
My reasonings are two - 1)regex is not the onl one who would use the user query, 2) the expansion may be usefull everywhere

replacement = splitJob[number];
}

converted = converted.replaceFirst("%\\{((N-)?[0-9]+|S|SPLIT)\\}", replacement);
Copy link
Contributor

Choose a reason for hiding this comment

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

Dont forget the replace is eager. I bet we will find (in unittest O:) ) something where the eagerness maybe bite us.

Otherwise the algortihm looks good. why is there the converted=converted.repalceFirst? I thought that the matcher will aloow you to do this.
if this line will persits, then you have to remove duplicated regex declaration.

@judovana
Copy link
Contributor

For many reasons I would liek to see the = x space refactoring as separate PR, but I guess that is a torture, right?-(

@patrikcerbak patrikcerbak changed the title Comparator Jenkins intergration and changes to Comparator arguments Comparator Jenkins intergration Oct 23, 2023
@patrikcerbak
Copy link
Collaborator Author

For many reasons I would liek to see the = x space refactoring as separate PR, but I guess that is a torture, right?-(

Ok, done. :)

@judovana judovana merged commit fc0c6dc into jenkinsci:master Oct 26, 2023
13 checks passed
@patrikcerbak patrikcerbak deleted the links-to-comparator branch October 30, 2023 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants