-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
Number of open issue probe #353
base: main
Are you sure you want to change the base?
Changes from 37 commits
2773dcf
68aa505
54a82a3
abb1001
4d8ae5d
c4c1605
99d9cf5
719c57d
925afa3
58b135a
c71a32a
5fd8b2d
6f9ede3
1a54932
4a144e5
d3fd18b
d960448
eda1b13
97aed28
528ab43
5f22f38
e13f3a8
9bcb49e
4989e81
7db7e7a
e45ce77
510d340
3cf7406
b9cda9c
37884d5
898c86c
fdd1a4e
1940e67
151df0d
12ef4aa
7f6cb04
6a7214a
8e0f12a
12db24b
793114b
e13b17e
f4ef76e
1e194e9
d9fad0c
663282c
c099e40
ec28408
6a740d2
9244d60
b722a87
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
/* | ||
* MIT License | ||
* | ||
* Copyright (c) 2023 Jenkins Infra | ||
* | ||
* Permission is hereby granted, free of charge, to any person obtaining a copy | ||
* of this software and associated documentation files (the "Software"), to deal | ||
* in the Software without restriction, including without limitation the rights | ||
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell | ||
* copies of the Software, and to permit persons to whom the Software is | ||
* furnished to do so, subject to the following conditions: | ||
* | ||
* The above copyright notice and this permission notice shall be included in all | ||
* copies or substantial portions of the Software. | ||
* | ||
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | ||
* SOFTWARE. | ||
*/ | ||
|
||
package io.jenkins.pluginhealth.scoring.probes; | ||
|
||
import java.util.Optional; | ||
|
||
import io.jenkins.pluginhealth.scoring.model.Plugin; | ||
import io.jenkins.pluginhealth.scoring.model.ProbeResult; | ||
|
||
public abstract class AbstractOpenIssuesProbe extends Probe { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey @Jagrutiti, There is something wrong with that class which comes from its design. On purpose, I won't give you more indication for now, to let you figure it out by yourself. Please have a look at other similar classes in the project, and please read more about the purpose of You already implemented similar kind of classes in previous PRs, but perhaps the extra guidance we provided made that you did not get to understand the reason why we recommended this design, which is why this time I won't share much details (yet) and let you take some time to think about the design choices and understand the reason for them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @aneveux , I did read about implementing an The articles spoke of data abstraction and functional abstraction. In our case I believe, we are doing functional abstraction. Having two different methods is important here because the implementation to count issues from both I had done a similar implementation in The next point that I come across is why
The abstract classes that I implemented did provide code reusability and their functionalities were hidden from the child class. They only had to implement the abstract class. In the About the design issues, Maybe I should change the Maybe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Let me just say, what if tomorrow there is a third issue tracker used in the Jenkins project, what would we have to add or changed in here to make it fit? In OOP, it should require a minimal addition, very specific. If you look at the DependencyBot abstract class, how are the different bot handled? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey @Jagrutiti 👋 Sorry for the late reply, I'm out of the office right now and spend less time on the computer 😝 Adrien actually gave a nice indication regarding the design: what happens if you had to add a third issue tracker? The idea behind code abstraction is that the An That being said though, maybe while implementing your code you will notice that there is nothing to share between the two classes that you write because they are too different from each other, and in that case, it would be better to challenge the need for abstraction, and simply write 2 classes that are doing their job. Introducing abstraction just for the sake of it just makes the source code more complicated, hence less readable. It is tempting to want to use good practices like abstraction while writing code when we manipulate things that seem similar, but it is important to check that they are indeed similar. Hope that helps you understand my concern with the class design. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for explaining @aneveux. In past cases when I used In this case, there was no common code. Maybe I should have asked this in the meeting itself but it did not occur to me then. I should have taken inspiration from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's alright @Jagrutiti =) We learn by making mistakes, and if you never experienced such a case before, it would have been difficult to think about it and ask a question during the meeting. If you understand it better now, then that's great =) |
||
public static final int ORDER = IssueTrackerDetectionProbe.ORDER + 100; | ||
|
||
@Override | ||
protected ProbeResult doApply(Plugin plugin, ProbeContext context) { | ||
final Optional<Integer> openIssuesCount = getCountOfOpenIssues(context); | ||
return openIssuesCount.isPresent() | ||
? ProbeResult.success(key(), String.format("%d open issues found in the %s plugin.", openIssuesCount.get(), plugin.getName())) | ||
: ProbeResult.failure(key(), String.format("Could not find open issues in the %s plugin.", plugin.getName())); | ||
} | ||
|
||
abstract Optional<Integer> getCountOfOpenIssues(ProbeContext context); | ||
|
||
@Override | ||
public String[] getProbeResultRequirement() { | ||
return new String[] { SCMLinkValidationProbe.KEY, IssueTrackerDetectionProbe.KEY }; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
/* | ||
* MIT License | ||
* | ||
* Copyright (c) 2023 Jenkins Infra | ||
* | ||
* Permission is hereby granted, free of charge, to any person obtaining a copy | ||
* of this software and associated documentation files (the "Software"), to deal | ||
* in the Software without restriction, including without limitation the rights | ||
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell | ||
* copies of the Software, and to permit persons to whom the Software is | ||
* furnished to do so, subject to the following conditions: | ||
* | ||
* The above copyright notice and this permission notice shall be included in all | ||
* copies or substantial portions of the Software. | ||
* | ||
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | ||
* SOFTWARE. | ||
*/ | ||
|
||
package io.jenkins.pluginhealth.scoring.probes; | ||
|
||
import java.io.IOException; | ||
import java.util.Optional; | ||
|
||
import org.kohsuke.github.GHRepository; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
import org.springframework.core.annotation.Order; | ||
import org.springframework.stereotype.Component; | ||
|
||
@Component | ||
@Order(AbstractOpenIssuesProbe.ORDER) | ||
class GitHubOpenIssuesProbe extends AbstractOpenIssuesProbe { | ||
public static final String KEY = "github-open-issues"; | ||
private static final Logger LOGGER = LoggerFactory.getLogger(GitHubOpenIssuesProbe.class); | ||
|
||
/** | ||
* Get total number of open GitHub issues in a plugin. | ||
* | ||
* @param context {@link ProbeContext} | ||
* @return Optional an Integer value will give total count of open issues. | ||
*/ | ||
@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"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If you are not sure, you can add a first check to validate that the plugin has an issue tracker configured? (a simple There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It is better to be safe than sorry. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the "better safe than sorry" principal. 👍 |
||
|
||
if (issueTrackerViewUrl == null) { | ||
LOGGER.info("The plugin does not use GitHub issues to tracker issues."); | ||
Jagrutiti marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return Optional.empty(); | ||
} | ||
|
||
try { | ||
final Optional<String> repositoryName = context.getRepositoryName(issueTrackerViewUrl.substring(0, issueTrackerViewUrl.lastIndexOf("/"))); | ||
if (repositoryName.isPresent()) { | ||
final GHRepository ghRepository = context.getGitHub().getRepository(repositoryName.get()); | ||
return Optional.of(ghRepository.getOpenIssueCount()); | ||
} | ||
} catch (IOException ex) { | ||
LOGGER.error("Cannot read open issues on GitHub for the plugin. {}", ex); | ||
Jagrutiti marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
return Optional.empty(); | ||
} | ||
|
||
@Override | ||
public String key() { | ||
return KEY; | ||
} | ||
|
||
@Override | ||
public String getDescription() { | ||
return "Returns the total number of open issues in GitHub."; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
/* | ||
* MIT License | ||
* | ||
* Copyright (c) 2023 Jenkins Infra | ||
* | ||
* Permission is hereby granted, free of charge, to any person obtaining a copy | ||
* of this software and associated documentation files (the "Software"), to deal | ||
* in the Software without restriction, including without limitation the rights | ||
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell | ||
* copies of the Software, and to permit persons to whom the Software is | ||
* furnished to do so, subject to the following conditions: | ||
* | ||
* The above copyright notice and this permission notice shall be included in all | ||
* copies or substantial portions of the Software. | ||
* | ||
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | ||
* SOFTWARE. | ||
*/ | ||
|
||
package io.jenkins.pluginhealth.scoring.probes; | ||
|
||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.stream.Collectors; | ||
|
||
import io.jenkins.pluginhealth.scoring.model.Plugin; | ||
import io.jenkins.pluginhealth.scoring.model.ProbeResult; | ||
import io.jenkins.pluginhealth.scoring.model.updatecenter.Plugin.IssueTrackers; | ||
import io.jenkins.pluginhealth.scoring.model.updatecenter.UpdateCenter; | ||
|
||
import org.springframework.core.annotation.Order; | ||
import org.springframework.stereotype.Component; | ||
|
||
@Component | ||
@Order(value = IssueTrackerDetectionProbe.ORDER) | ||
class IssueTrackerDetectionProbe extends Probe { | ||
public static final String KEY = "issue-tracker-detection"; | ||
public static final int ORDER = UpdateCenterPluginPublicationProbe.ORDER + 100; | ||
|
||
@Override | ||
protected ProbeResult doApply(Plugin plugin, ProbeContext context) { | ||
Map<String, String> issueTrackerDetails = getIssueTrackersForAPlugin(plugin.getName(), context.getUpdateCenter()); | ||
|
||
if (issueTrackerDetails.isEmpty()) { | ||
return ProbeResult.failure(key(), String.format("No issue tracker data available for %s plugin in Update Center.", plugin.getName())); | ||
} | ||
context.setIssueTrackerNameAndUrl(issueTrackerDetails); | ||
return ProbeResult.success(key(), "Issue tracker detected and returned successfully."); | ||
Jagrutiti marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
@Override | ||
public String[] getProbeResultRequirement() { | ||
return new String[]{UpdateCenterPluginPublicationProbe.KEY}; | ||
} | ||
|
||
@Override | ||
public String key() { | ||
return KEY; | ||
} | ||
|
||
@Override | ||
public String getDescription() { | ||
return "Detects the issues tracker type from Update Center."; | ||
} | ||
|
||
/** | ||
* Gets issue trackers for a specific plugin from Update Center. | ||
* | ||
* @param pluginName name of the plugin to fetch issue tracker data for. | ||
* @param updateCenter The {@link UpdateCenter}. | ||
* @return A Map of filtered data from issue trackers. | ||
*/ | ||
private Map<String, String> getIssueTrackersForAPlugin(String pluginName, UpdateCenter updateCenter) { | ||
return updateCenter.plugins().get(pluginName) != null | ||
? filterIssueTrackersForTypeAndViewUrl(updateCenter.plugins().get(pluginName).getIssueTrackers()) | ||
Jagrutiti marked this conversation as resolved.
Show resolved
Hide resolved
Jagrutiti marked this conversation as resolved.
Show resolved
Hide resolved
|
||
: Map.of(); | ||
} | ||
|
||
/** | ||
* Filters IssueTrackers for "type" and "viewUrl". | ||
* | ||
* @param issueTrackers Accepts a list of {@link IssueTrackers}. | ||
* @return A Map of {@code IssueTrackers::type} and {@code IssueTrackers::viewUrl}. | ||
*/ | ||
private Map<String, String> filterIssueTrackersForTypeAndViewUrl(List<IssueTrackers> issueTrackers) { | ||
return issueTrackers.stream() | ||
.collect(Collectors.toMap(IssueTrackers::type, IssueTrackers::viewUrl)); | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's your rationale for using this Apache httpclient instead of the Java HttpClient for example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apache HTTPClient is an additional dependency. While I can do the same thing using Java HTTPClient. So I shouldn't be using it.