Number of open issue probe #353

Jagrutiti · 2023-07-23T16:38:37Z

Description

This probe fetches the number of open issues in a plugin.

Steps:

Create IssueTrackerDetectionProbe, this probe will check whether JIRA, GitHub is configured or both is configured.

Return a success as long as at least one issue tracker configured.
If no issue tracker is configured that is an error
more than one error is a success. Stores which ones are configured.

For GitHubOpenIssuesProbe and JiraOpenIssuesProbe:

Create an abstract class. That would be the parent for GitHubOpenIssuesProbe and JiraOpenIssuesProbe.
Make sure that IssueTrackerDetectionProbe is executed and success is returned.
Get the message of the probe and pass it. Try to see which system is used.
Return a Map<key, url>

Some code reusing details:

Basically this from your PR:

       return context.getUpdateCenter()
           .issueTrackers().stream()
           .flatMap(map -> map.entrySet().stream())
           .filter(map -> map.getKey().equals(filter))
           .map(map -> map.getValue())
           .collect(Collectors.toList());
   }

Is your entry point for the new probe

IssueTrackerDetectionProbe
| - GitHubOpenIssuesProbe
| - JiraOpemIssuesProbe

"github" - "https://github.com/
"jira" - "https://issues.jenkins.io

Closes #143

Testing done

Submitter checklist

Give feedback

If an issue exists, it is well described and linked in the description
The description of this pull request is detailed and explain why this pull request is needed
The changeset is on a specific branch. Using feature/ for new feature, or improvements ; Using fix/ for bug fixes ; Using docs/ for any documentation changes.
If required, the documentation has been updated
There is automated tests to cover the code change / addition or an explanation why there is no tests in the description.
Options

core/src/main/java/io/jenkins/pluginhealth/scoring/model/Plugin.java

…new ones

core/src/test/java/io/jenkins/pluginhealth/scoring/probes/NumberOfOpenIssuesProbeTest.java

core/src/main/java/io/jenkins/pluginhealth/scoring/probes/NumberOfOpenIssuesProbe.java

core/src/test/java/io/jenkins/pluginhealth/scoring/probes/NumberOfOpenIssuesProbeTest.java

core/src/main/java/io/jenkins/pluginhealth/scoring/probes/NumberOfOpenIssuesProbe.java

aneveux

Did not get time to finish a full review, but already sharing bunch of comments

core/src/main/java/io/jenkins/pluginhealth/scoring/probes/NumberOfOpenIssuesProbe.java

core/src/main/java/io/jenkins/pluginhealth/scoring/model/updatecenter/UpdateCenter.java

core/src/main/java/io/jenkins/pluginhealth/scoring/probes/IssueTrackerDetectionProbe.java

Jagrutiti · 2023-08-14T15:38:03Z

Your reviews help me learn how can I do the same thing in a better way. Thank you for your time.

if you'd like me to do it some other way, just let me know.

I am fine with the current method. It helps me clearly see the line that should be improved and fix similar issues.

I read all the reviews twice, took my time, and then tackled them one by one. This way I understood the bigger picture and avoided the mistakes I made in the past.

…ti/plugin-health-scoring into feature/open-issue-probe

aneveux

Few comments from a quick review. General remark: adding Javadocs on the classes you write is also interesting to understand the rationale for classes to exist. It is often a nice thing to read as a first discovery of what a class is for.

aneveux · 2023-08-18T07:55:13Z

core/pom.xml

+		<!-- Used to invoke API calls  -->
+		<dependency>
+			<groupId>org.apache.httpcomponents</groupId>
+			<artifactId>httpclient</artifactId>
+			<version>4.5.14</version>
+		</dependency>


What's your rationale for using this Apache httpclient instead of the Java HttpClient for example?

Apache HTTPClient is an additional dependency. While I can do the same thing using Java HTTPClient. So I shouldn't be using it.

core/src/main/java/io/jenkins/pluginhealth/scoring/probes/GitHubOpenIssuesProbe.java

aneveux · 2023-08-18T08:03:35Z

core/src/main/java/io/jenkins/pluginhealth/scoring/probes/JiraOpenIssuesProbe.java

+        String viewJiraIssuesUrl = context.getIssueTrackerUrlsByNames().get("jira");
+
+        if (viewJiraIssuesUrl == null || viewJiraIssuesUrl.isEmpty()) {
+            LOGGER.info("JIRA issues not found in Update Center for the plugin.");


For consistency, it would be better to use a similar message than the one used for the same situation with GitHub issues:

The plugin does not use GitHub issues to tracker issues.

(talking about Jira of course)

aneveux · 2023-08-18T15:10:40Z

core/src/main/java/io/jenkins/pluginhealth/scoring/probes/JiraOpenIssuesProbe.java

+
+            httpRequest = HttpRequest.newBuilder()
+                .uri(new URI(api))
+                .timeout(Duration.of(5, SECONDS))


Isn't this too quick? What is the default value?

IMHO, for any decision like this, it is interesting that you leave a simple comment in the source code explaining why you took that decision, so readers can understand if it is a deliberate choice, and what is the rationale for it.

General remark: remember you mostly write code for other humans, so you need to tell them your intention.

The JavaDoc says:

The effect of not setting a timeout is the same as setting an infinite Duration, i.e. block forever.

I was not really sure which value to pick. Honestly, the APIs I had tested took less than a second to respond. So I figured 5 seconds is apt.

What should I actually write as the comment?

What should I actually write as the comment?

The general rule to follow while writing source code is that someone reading it should not have to guess why you did certain things, but should find the information written explicitely.

If you thought that 5 seconds was a good timeout value based on your testing, then write it in a simple comment, so it is now our shared understanding.

Basically, imagine reading that piece of source code if you did not write it yourself, and try to see if you understand everything from what you read.

aneveux · 2023-08-18T15:12:40Z

core/src/test/java/io/jenkins/pluginhealth/scoring/probes/CodeCoverageProbeTest.java

@@ -112,7 +112,7 @@ public void shouldBeInErrorWhenRepositoryIsNotInOrganization() {

        when(ctx.getUpdateCenter()).thenReturn(new UpdateCenter(
            Map.of(
-                pluginName, new io.jenkins.pluginhealth.scoring.model.updatecenter.Plugin(
+                pluginName, io.jenkins.pluginhealth.scoring.model.updatecenter.Plugin.of(


Looks like a lot of those Plugin objects are the same, with similar values. Maybe extract that in a variable that can be reused everywhere?

Here parameters like pluginName, scmLink, defaultBranch are mocked before being passed.

You do not mean creating a constructor in Record like I already did it once?

That's OK like this

…ubOpenIssuesProbe.java Co-authored-by: Antoine Neveux <[email protected]>

aneveux

Very minor comments. Great job Jagruti!

Just few little things to clean-up and we'll be ready to merge.

aneveux · 2023-08-22T12:55:33Z

core/src/main/java/io/jenkins/pluginhealth/scoring/probes/GitHubOpenIssuesProbe.java

+    @Override
+    Optional<Integer> getCountOfOpenIssues(ProbeContext context) {
+        /* Stores the GitHub URL to view all existing issues in the plugin. Ex: https://github.com/jenkinsci/cloudevents-plugin/issues */
+        String issueTrackerViewUrl = context.getIssueTrackerUrlsByNames().get("github");


context.getIssueTrackerUrlsByNames() cannot be null right?

If you are not sure, you can add a first check to validate that the plugin has an issue tracker configured? (a simple if before trying to .get("github");)

IssueTrackerDetectionProbe is the requirement for GitHubOpenIssuesProbe and UpdateCenterPluginPublicationProbe is the requirement for IssueTrackerDetectionProbe that is why a part of my believes that this issue might not occur.

It is better to be safe than sorry.

I like the "better safe than sorry" principal. 👍

core/src/main/java/io/jenkins/pluginhealth/scoring/probes/GitHubOpenIssuesProbe.java

core/src/main/java/io/jenkins/pluginhealth/scoring/probes/IssueTrackerDetectionProbe.java

aneveux · 2023-08-22T13:04:01Z

core/src/main/java/io/jenkins/pluginhealth/scoring/probes/JiraOpenIssuesProbe.java

+        String viewJiraIssuesUrl = context.getIssueTrackerUrlsByNames().get("jira");
+
+        if (viewJiraIssuesUrl == null || viewJiraIssuesUrl.isEmpty()) {
+            LOGGER.info("The plugin does not use JIRA to track issues.");


Same remark: which plugin?

I don't think this was addressed.

This requires the plugin to be passed as a parameter to getCountOfOpenIssues to get plugin.getName() that is why
did not work on this one.

or you could just provide the name of the plugin, rather than the entire plugin.

or you could just provide the name of the plugin, rather than the entire plugin.

I did something similar last time. You said we shouldn't pass a parameter just to display in the logs, But if you are cool with it I can do that,

core/src/main/java/io/jenkins/pluginhealth/scoring/probes/JiraOpenIssuesProbe.java

aneveux · 2023-08-22T13:08:00Z

core/src/test/java/io/jenkins/pluginhealth/scoring/probes/CodeCoverageProbeTest.java

@@ -112,7 +112,7 @@ public void shouldBeInErrorWhenRepositoryIsNotInOrganization() {

        when(ctx.getUpdateCenter()).thenReturn(new UpdateCenter(
            Map.of(
-                pluginName, new io.jenkins.pluginhealth.scoring.model.updatecenter.Plugin(
+                pluginName, io.jenkins.pluginhealth.scoring.model.updatecenter.Plugin.of(


That's OK like this

core/src/test/java/io/jenkins/pluginhealth/scoring/probes/GitHubOpenIssuesProbeTest.java

core/src/main/java/io/jenkins/pluginhealth/scoring/model/updatecenter/Plugin.java

…ubOpenIssuesProbeTest.java Co-authored-by: Antoine Neveux <[email protected]>

…ti/plugin-health-scoring into feature/open-issue-probe

core/src/main/java/io/jenkins/pluginhealth/scoring/probes/GitHubOpenIssuesProbe.java

core/src/main/java/io/jenkins/pluginhealth/scoring/probes/JiraOpenIssuesProbe.java

aneveux

LGTM.

I'll let @alecharp have another look at it in case I missed something, and I believe that'll be ready to merge 🚀

alecharp

there are a few things that were not addressed.

I would like to know why you moved the getSpy method implementation on most (if not all) test classes.

core/src/main/java/io/jenkins/pluginhealth/scoring/model/updatecenter/Plugin.java

alecharp · 2023-09-06T09:16:50Z

core/src/main/java/io/jenkins/pluginhealth/scoring/probes/GitHubOpenIssuesProbe.java

+    @Override
+    Optional<Integer> getCountOfOpenIssues(ProbeContext context) {
+        /* Stores the GitHub URL to view all existing issues in the plugin. Ex: https://github.com/jenkinsci/cloudevents-plugin/issues */
+        String issueTrackerViewUrl = context.getIssueTrackerUrlsByNames().get("github");


I like the "better safe than sorry" principal. 👍

core/src/main/java/io/jenkins/pluginhealth/scoring/probes/IssueTrackerDetectionProbe.java

alecharp · 2023-09-06T09:18:58Z

core/src/main/java/io/jenkins/pluginhealth/scoring/probes/JiraOpenIssuesProbe.java

+    HttpClient httpClient;
+    HttpRequest httpRequest;


I agree here. those fields should be local variables.

alecharp · 2023-09-06T09:19:38Z

core/src/main/java/io/jenkins/pluginhealth/scoring/probes/JiraOpenIssuesProbe.java

+     */
+    @Override
+    Optional<Integer> getCountOfOpenIssues(ProbeContext context) {
+        String viewJiraIssuesUrl = context.getIssueTrackerUrlsByNames().get("jira");


Same as before, "better safe than sorry".

core/src/test/java/io/jenkins/pluginhealth/scoring/probes/JiraOpenIssuesProbeTest.java

alecharp · 2023-09-06T09:34:33Z

...rc/test/java/io/jenkins/pluginhealth/scoring/probes/KnownSecurityVulnerabilityProbeTest.java

+    @Override
+    KnownSecurityVulnerabilityProbe getSpy() {
+        return spy(KnownSecurityVulnerabilityProbe.class);
+    }
+


Same as before.

alecharp · 2023-09-06T09:36:18Z

core/src/test/java/io/jenkins/pluginhealth/scoring/probes/SpotBugsProbeTest.java

+    @Override
+    protected SpotBugsProbe getSpy() {
+        return spy(SpotBugsProbe.class);
+    }
+


It is all because of Ctrl + Alt + L. I did not do it manually.

alecharp · 2023-09-06T09:36:27Z

core/src/test/java/io/jenkins/pluginhealth/scoring/probes/UpForAdoptionProbeTest.java

+    @Override
+    UpForAdoptionProbe getSpy() {
+        return spy(UpForAdoptionProbe.class);
+    }
+


alecharp · 2023-09-06T09:36:37Z

...test/java/io/jenkins/pluginhealth/scoring/probes/UpdateCenterPluginPublicationProbeTest.java

+    @Override
+    UpdateCenterPluginPublicationProbe getSpy() {
+        return spy(UpdateCenterPluginPublicationProbe.class);
+    }
+


Jagrutiti · 2023-09-06T10:43:02Z

there are a few things that were not addressed.

I would like to know why you moved the getSpy method implementation on most (if not all) test classes.

All I did was Ctrl + Alt + L. It moved because of formatting.

Co-authored-by: Adrien Lecharpentier <[email protected]>

alecharp · 2024-04-02T09:36:14Z

Is this still something you want to contribute @Jagrutiti?

Jagrutiti · 2024-04-04T04:35:34Z

Is this still something you want to contribute @Jagrutiti?

Yes. I am interested.

I remember this PR wasn't merged because of some code structuring thing and variable scope used in the test cases that you wanted to refactor.

alecharp · 2024-10-18T13:42:05Z

@Jagrutiti do you think you will be able to complete this pull request?

Fetched issue tracker from update center

2773dcf

Jagrutiti self-assigned this Jul 23, 2023

Jagrutiti changed the title ~~Open issue probe~~ Number of open issue probe Jul 23, 2023

alecharp requested changes Jul 24, 2023

View reviewed changes

core/src/main/java/io/jenkins/pluginhealth/scoring/model/Plugin.java Outdated Show resolved Hide resolved

Jagrutiti added 9 commits July 24, 2023 14:33

Removed issueTracker from models

68aa505

Modified the existing test cases that were affected and updated then …

54a82a3

…new ones

Fixing checkstyle issues

abb1001

Fixing checkstyle issues

4d8ae5d

Updated probe names and added java docs

c4c1605

Added test and code to count open GitHub issues

99d9cf5

Added test code and completed the test case

719c57d

Removed conficting dependencies

925afa3

Fixed incorrect artifact name

58b135a

Jagrutiti marked this pull request as ready for review July 30, 2023 15:15

Merge branch 'main' into feature/open-issue-probe

c71a32a

Jagrutiti requested review from alecharp, aneveux and PierreBtz July 30, 2023 15:16

Jagrutiti commented Jul 30, 2023

View reviewed changes

core/src/test/java/io/jenkins/pluginhealth/scoring/probes/NumberOfOpenIssuesProbeTest.java Outdated Show resolved Hide resolved

Jagrutiti commented Jul 30, 2023

View reviewed changes

core/src/test/java/io/jenkins/pluginhealth/scoring/probes/NumberOfOpenIssuesProbeTest.java Outdated Show resolved Hide resolved

alecharp requested changes Jul 31, 2023

View reviewed changes

Fixed JIRA API calling

5fd8b2d

aneveux reviewed Aug 1, 2023

View reviewed changes

Jagrutiti added 2 commits August 1, 2023 21:41

Merge branch 'main' into feature/open-issue-probe

6f9ede3

Added IssueTrackerDetectionProbe

1a54932

alecharp reviewed Aug 3, 2023

View reviewed changes

core/src/main/java/io/jenkins/pluginhealth/scoring/model/updatecenter/UpdateCenter.java Outdated Show resolved Hide resolved

alecharp reviewed Aug 3, 2023

View reviewed changes

core/src/main/java/io/jenkins/pluginhealth/scoring/probes/IssueTrackerDetectionProbe.java Outdated Show resolved Hide resolved

Jagrutiti marked this pull request as draft August 3, 2023 14:23

Jagrutiti added 2 commits August 5, 2023 12:27

Update IssueTrackerDetectionProbe and added a test for the same

4a144e5

Added abstract probe class and jira and github issues count probe

d3fd18b

Jagrutiti added 4 commits August 14, 2023 21:08

Merge branch 'main' into feature/open-issue-probe

151df0d

Removed unused imports

12ef4aa

Merge branch 'feature/open-issue-probe' of https://github.com/Jagruti…

7f6cb04

…ti/plugin-health-scoring into feature/open-issue-probe

Fixed JavaDocs

6a7214a

aneveux reviewed Aug 18, 2023

View reviewed changes

Jagrutiti and others added 3 commits August 19, 2023 19:52

Update core/src/main/java/io/jenkins/pluginhealth/scoring/probes/GitH…

8e0f12a

…ubOpenIssuesProbe.java Co-authored-by: Antoine Neveux <[email protected]>

Removed unwanted dependencies and fixed log statements

12db24b

Adding a comment

793114b

aneveux reviewed Aug 22, 2023

View reviewed changes

alecharp reviewed Aug 23, 2023

View reviewed changes

core/src/main/java/io/jenkins/pluginhealth/scoring/model/updatecenter/Plugin.java Outdated Show resolved Hide resolved

Jagrutiti and others added 3 commits August 23, 2023 22:28

Update core/src/test/java/io/jenkins/pluginhealth/scoring/probes/GitH…

e13b17e

…ubOpenIssuesProbeTest.java Co-authored-by: Antoine Neveux <[email protected]>

Made minor changes based on code review

f4ef76e

Merge branch 'feature/open-issue-probe' of https://github.com/Jagruti…

1e194e9

…ti/plugin-health-scoring into feature/open-issue-probe

aneveux reviewed Aug 24, 2023

View reviewed changes

core/src/main/java/io/jenkins/pluginhealth/scoring/probes/GitHubOpenIssuesProbe.java Outdated Show resolved Hide resolved

aneveux reviewed Aug 24, 2023

View reviewed changes

core/src/main/java/io/jenkins/pluginhealth/scoring/probes/JiraOpenIssuesProbe.java Outdated Show resolved Hide resolved

Adding null checks

d9fad0c

aneveux previously approved these changes Aug 27, 2023

View reviewed changes

alecharp requested changes Sep 6, 2023

View reviewed changes

Jagrutiti and others added 6 commits September 6, 2023 20:37

Apply suggestions from code review

663282c

Co-authored-by: Adrien Lecharpentier <[email protected]>

Fixed foramatting, updated library uses based on code review

c099e40

Updated syntax for null check

ec28408

Merge branch 'main' into feature/open-issue-probe

6a740d2

Fixing merge issues

9244d60

Fixing checkstyle issues

b722a87

Jagrutiti dismissed aneveux’s stale review via b722a87 September 26, 2023 09:44

Number of open issue probe #353

Are you sure you want to change the base?

Number of open issue probe #353

Conversation

Jagrutiti commented Jul 23, 2023 • edited Loading

Description

Testing done

Submitter checklist

aneveux left a comment

Choose a reason for hiding this comment

Jagrutiti commented Aug 14, 2023

aneveux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aneveux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aneveux left a comment

Choose a reason for hiding this comment

alecharp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jagrutiti commented Sep 6, 2023

alecharp commented Apr 2, 2024

Jagrutiti commented Apr 4, 2024

alecharp commented Oct 18, 2024

Jagrutiti commented Jul 23, 2023 •

edited

Loading