Validate GitHub scm source #34

XiongKezhi · 2020-08-23T18:20:25Z

When creating publisher, check whether all ChecksContext parameters are available using isValid method before returning the GitHubChecksPublisher; if some parameters are not, return Optional.empty(), thus a NullPublisher will be used.

…hed checks.

timja · 2020-08-23T20:37:02Z

src/main/java/io/jenkins/plugins/checks/github/GitHubChecksPublisherFactory.java

-        PluginLogger logger = createLogger(getListener(listener));
+    Optional<ChecksPublisher> createPublisher(final Run<?, ?> run, final String runURL, final TaskListener listener)
+            throws UnsupportedEncodingException {
+        ByteArrayOutputStream cause = new ByteArrayOutputStream();


doesn't this need closing? i.e. add to try with resources?

timja · 2020-08-23T20:37:43Z

src/main/java/io/jenkins/plugins/checks/github/GitHubChecksPublisherFactory.java

-            final GitHubChecksContext gitHubSCMSourceContext) {
-        if (gitHubSCMSourceContext.isValid(logger)) {
-            return Optional.of(new GitHubChecksPublisher(gitHubSCMSourceContext, getListener(listener)));
+    @VisibleForTesting


agree with the check above about duplication can it be improved so we don't have 3 very similar methods?

src/main/java/io/jenkins/plugins/checks/github/GitHubChecksPublisher.java

uhafner · 2020-08-24T10:54:27Z

src/main/java/io/jenkins/plugins/checks/github/GitHubChecksPublisherFactory.java

+            return createPublisher(run, DisplayURLProvider.get().getRunURL(run), listener);
+        }
+        catch (UnsupportedEncodingException e) {
+            LOGGER.log(Level.WARNING, "Could not create logger.", e);


Is the LOGGER still required? We can use the PluginLogger that prints to the console log.

yes, we can, just I don't know which log the users normally use, the system log or console log?

I never look into the system log. I think if something goes wrong due to a wrong configuration it makes sense to be part of the console log. What do you think, @timja?

I can't really imagine why this would ever fail, I think it shouldn't be a checked exception, and then this goes away

uhafner · 2020-08-24T10:56:25Z

src/main/java/io/jenkins/plugins/checks/github/GitHubChecksPublisherFactory.java

+        try {
+            return createPublisher(run, DisplayURLProvider.get().getRunURL(run), listener);
+        }
+        catch (UnsupportedEncodingException e) {


Would it make sense to use a safe default if the URL is broken?

the UnsupportedEncodingException is thrown by the constructor of PrintStream, not the url.

uhafner · 2020-08-24T11:00:00Z

src/main/java/io/jenkins/plugins/checks/github/GitHubChecksPublisherFactory.java

+    Optional<ChecksPublisher> createPublisher(final Run<?, ?> run, final String runURL, final TaskListener listener)
+            throws UnsupportedEncodingException {
+        ByteArrayOutputStream cause = new ByteArrayOutputStream();
+        try (PrintStream ps = new PrintStream(cause, true, StandardCharsets.UTF_8.name())) {


Why are you creating a new PrintStream? Can't you use the TaskListener directly? See line 118.

Because when users are using GitHubSCMSource project, some logs like "no credentials available" for validating the GitSCMContext are useless and maybe annoying for them IMO, so I tried to just log these "causes" when no suitable publisher is found.

We can either place the GitHubSCM first (this has the same effect for GitSCM users) or we can use the FilteredLog for both context types and print the log only in case that both return nothing.

yes, I'm finding such a logger, I'll use FilteredLog.

uhafner · 2020-08-24T11:06:16Z

src/main/java/io/jenkins/plugins/checks/github/GitHubSCMSourceChecksContext.java

+
+    @Nullable
+    private String resolveHeadSha() {
+        if (runAvailable) {


Rather than using an if else here and adding a new field runAvailable it would be nicer if you would assign the SHA directly in the corresponding Job and Run constructors.

timja · 2020-08-24T14:59:44Z

src/main/java/io/jenkins/plugins/checks/github/GitHubChecksPublisherFactory.java

-    Optional<ChecksPublisher> createPublisher(final Job<?, ?> job, final String jobURL, final TaskListener listener) {
-        PluginLogger logger = createLogger(getListener(listener));
+    Optional<ChecksPublisher> createPublisher(final Run<?, ?> run, final String runURL, final TaskListener listener)
+            throws UnsupportedEncodingException {


what do you expect consumers to do here?
This doesn't seem like it would ever happen.

Better to remove it from the API and handle it internaly to the method I think

it is thrown by the constructor of PrintStream. Now I'll refactor and use the FilteredLog implemented by Ulli in his plugin util, then we don't have to construct those streams and this will be removed.

codecov · 2020-08-24T17:08:55Z

Codecov Report

Merging #34 into master will increase coverage by 2.00%.
The diff coverage is 88.60%.

@@             Coverage Diff              @@
##             master      #34      +/-   ##
============================================
+ Coverage     75.33%   77.33%   +2.00%     
- Complexity       94       96       +2     
============================================
  Files             7        7              
  Lines           300      300              
  Branches         32       33       +1     
============================================
+ Hits            226      232       +6     
+ Misses           52       48       -4     
+ Partials         22       20       -2

Impacted Files	Coverage Δ	Complexity Δ
...va/io/jenkins/plugins/checks/github/SCMFacade.java	`54.00% <0.00%> (-7.37%)`	`15.00 <0.00> (ø)`
...ins/plugins/checks/github/GitSCMChecksContext.java	`65.21% <66.66%> (+11.72%)`	`16.00 <6.00> (+2.00)`
...ins/plugins/checks/github/GitHubChecksContext.java	`100.00% <100.00%> (ø)`	`13.00 <6.00> (-1.00)`
...s/plugins/checks/github/GitHubChecksPublisher.java	`96.77% <100.00%> (+5.86%)`	`5.00 <1.00> (ø)`
...ns/checks/github/GitHubChecksPublisherFactory.java	`100.00% <100.00%> (+9.52%)`	`7.00 <6.00> (-4.00)`	⬆️
...ns/checks/github/GitHubSCMSourceChecksContext.java	`100.00% <100.00%> (+10.52%)`	`20.00 <17.00> (+5.00)`

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 37de36b...92ce6e0. Read the comment docs.

…tener.NULL.

XiongKezhi added 2 commits August 24, 2020 00:03

Use isValid method for GitHubSCMSourceChecksContext

09b483c

Avoid log unnecessary information to console when successfully publis…

68457d4

…hed checks.

XiongKezhi requested a review from a team August 23, 2020 18:20

timja reviewed Aug 23, 2020

View reviewed changes

XiongKezhi requested a review from uhafner August 24, 2020 07:43

Close stream resources

fedd2ba

XiongKezhi linked an issue Aug 24, 2020 that may be closed by this pull request

GitHub Check does not get updated/dismissed after rebuild #21

Closed

XiongKezhi mentioned this pull request Aug 24, 2020

Try resolve commit from run first #27

Closed

2 tasks

uhafner reviewed Aug 24, 2020

View reviewed changes

Assign sha in context's constructor

35b6ab8

timja reviewed Aug 24, 2020

View reviewed changes

XiongKezhi added 2 commits August 24, 2020 23:35

Refactor to avoid duplicate code

874bcbd

Use filtered log

8b440f0

XiongKezhi added 2 commits August 25, 2020 01:49

Add more test for GitHubSCMSourceChecksContextTest

3ed27c3

Fix PMD waring on creating console logger

67636fd

uhafner approved these changes Aug 24, 2020

View reviewed changes

XiongKezhi added 4 commits August 25, 2020 11:57

More tests for GitHubChecksContext

492974e

Remove check for null task listener since checks-api has used TaskLis…

b545853

…tener.NULL.

Merge branch 'master' into validate-github-scm-source

cb23789

Clean up works after merge master.

92ce6e0

XiongKezhi merged commit a0c5db0 into master Aug 25, 2020

XiongKezhi deleted the validate-github-scm-source branch August 25, 2020 05:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate GitHub scm source #34

Validate GitHub scm source #34

XiongKezhi commented Aug 23, 2020

timja Aug 23, 2020

timja Aug 23, 2020

uhafner Aug 24, 2020

XiongKezhi Aug 24, 2020

uhafner Aug 24, 2020 •

edited

Loading

timja Aug 24, 2020

uhafner Aug 24, 2020

XiongKezhi Aug 24, 2020

uhafner Aug 24, 2020

XiongKezhi Aug 24, 2020 •

edited

Loading

uhafner Aug 24, 2020

XiongKezhi Aug 24, 2020

uhafner Aug 24, 2020

timja Aug 24, 2020

XiongKezhi Aug 24, 2020 •

edited

Loading

codecov bot commented Aug 24, 2020 •

edited

Loading

Validate GitHub scm source #34

Validate GitHub scm source #34

Conversation

XiongKezhi commented Aug 23, 2020

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

uhafner Aug 24, 2020 • edited Loading

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

XiongKezhi Aug 24, 2020 • edited Loading

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

XiongKezhi Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Aug 24, 2020 • edited Loading

Codecov Report

uhafner Aug 24, 2020 •

edited

Loading

XiongKezhi Aug 24, 2020 •

edited

Loading

XiongKezhi Aug 24, 2020 •

edited

Loading

codecov bot commented Aug 24, 2020 •

edited

Loading