Don't store actions on jobs #99

mrginglymus · 2020-12-27T14:30:43Z

No description provided.

codecov · 2020-12-27T15:54:11Z

Codecov Report

Merging #99 (735772d) into master (f9c6e96) will increase coverage by 0.01%.
The diff coverage is 93.75%.

@@             Coverage Diff              @@
##             master      #99      +/-   ##
============================================
+ Coverage     82.55%   82.56%   +0.01%     
- Complexity      145      149       +4     
============================================
  Files            13       13              
  Lines           430      436       +6     
  Branches         43       43              
============================================
+ Hits            355      360       +5     
- Misses           56       57       +1     
  Partials         19       19

Impacted Files	Coverage Δ	Complexity Δ
...ins/plugins/checks/github/GitSCMChecksContext.java	`62.00% <0.00%> (-1.27%)`	`16.00 <0.00> (ø)`
...ins/plugins/checks/github/GitHubChecksContext.java	`100.00% <100.00%> (ø)`	`20.00 <5.00> (+2.00)`
...ns/checks/github/GitHubChecksPublisherFactory.java	`100.00% <100.00%> (ø)`	`7.00 <1.00> (ø)`
...ns/checks/github/GitHubSCMSourceChecksContext.java	`100.00% <100.00%> (ø)`	`22.00 <4.00> (+2.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 f9c6e96...735772d. Read the comment docs.

XiongKezhi · 2020-12-27T16:16:36Z

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

@@ -45,7 +48,8 @@
     */
    GitHubSCMSourceChecksContext(final Job<?, ?> job, final String jobURL, final SCMFacade scmFacade) {
        super(job, jobURL, scmFacade);
-        sha = resolveHeadSha(job);
+        this.run = null;


This PMD warning is annoying 😤

Since we don't have a null object pattern for the run, I can't find a way to solve this other than shooting the message.

I could make run a Optional<Run<?, ?>>, but I gather that is frowned upon too?

Yes, right! I always thought possessing an optional inside a class makes another warning, can't remember where I saw this...

Well IntelliJ doesn’t like it, and IntelliJ is right about everything 😉

Well, I think the warning makes sense here. We introduced two constructors to avoid the problem of assigning null to the run. If you keep it this way you can remove the constructor as well

I guess we could have static fromRun and fromJob methods...

@uhafner would you prefer something like:

static GitHubSCMSourceChecksContext fromRun(final Run<?, ?> run, final String runURL, final SCMFacade scmFacade) { return new GitHubSCMSourceChecksContext(run.getParent(), run, runURL, scmFacade); } static GitHubSCMSourceChecksContext fromJob(final Job<?, ?> job, final String runURL, final SCMFacade scmFacade) { return new GitHubSCMSourceChecksContext(job, null, runURL, scmFacade); } /** * Creates a {@link GitHubSCMSourceChecksContext} according to the run. All attributes are computed during this period. * * @param job * a GitHub Branch Source project * @param run * a run of a GitHub Branch Source project * @param runURL * the URL to the Jenkins run * @param scmFacade * a facade for Jenkins SCM */ private GitHubSCMSourceChecksContext(final Job<?, ?> job, @CheckForNull final Run<?, ?> run, final String runURL, final SCMFacade scmFacade) { super(job, runURL, scmFacade); this.run = run; this.sha = Optional.ofNullable(run).map(this::resolveHeadSha).orElse(resolveHeadSha(job)); }

That moves (and duplicates) the logic for creating the context from a run into the calling code, which I don't think looks as nice.

it's ok IMO

I think we don't have to have the static methods, after all, the context is only used inside this package; they just add up the complexity.

The advantage behind having this way (preserved from having two constructors) is that it greatly reduces the chances of creating a GitHubSCMSourceChecksContext where the run does not belong to the job. I appreciate, as you say, that it's only used within the package but it still feels like a smell to have to duplicate the run.getParent(), run logic throughout the test code.

mrginglymus · 2020-12-27T16:21:14Z

Hmm, odd that it hasn't failed the run?

XiongKezhi · 2020-12-27T16:38:03Z

Hmm, odd that it hasn't failed the run?

Because there's a default threshold for PMD inherited in our pipeline step.

timja · 2020-12-28T09:27:19Z

Hmm, odd that it hasn't failed the run?

Because there's a default threshold for PMD inherited in our pipeline step.

that doesn't mean we should merge it with a known warning, it just means someone else will end up needing to fix it later

mrginglymus · 2020-12-28T10:09:03Z

There's something odd going on with failsafe - it's reporting the newly parametrized GitHubChecksPublisherITest without a package (so it now appears in (root)), nor with any of the parameters in the testcase name (so you can't tell which ones failed).

edit: ah, it appears to be a known issue scheduled for fixing in M6.

uhafner · 2020-12-27T23:03:59Z

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

@@ -38,6 +38,11 @@
        this.run = run;
    }

+    @Override
+    protected Optional<Run<?, ?>> getRun() {


Where is this new method consumed? Maybe we should better use the tell don't ask pattern.

It's used in getId, getAction and addActionIfMissing on GitHubChecksContext.

I did initially consider moving the Run member up to the parent GitHubChecksContext, but marking it as nullable would mean a lot of unnecessary null checks in GitSCMChecksContext, where it's known to never be null.

uhafner · 2020-12-28T11:38:58Z

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

@@ -45,7 +48,8 @@
     */
    GitHubSCMSourceChecksContext(final Job<?, ?> job, final String jobURL, final SCMFacade scmFacade) {
        super(job, jobURL, scmFacade);
-        sha = resolveHeadSha(job);
+        this.run = null;


In the beginning of the project we had one constructor for this class that sets all not used values to null, so if it makes sense we can combine the two constructors.

XiongKezhi

LGTM now.

Maybe I need to revisit the design of context sometime, it looks like the run is the norm (which should be held in the base class) and the job is an exception which is only used in our queue listener. @uhafner do you think so?

mrginglymus force-pushed the freestyle branch from 881b235 to 2b3f687 Compare December 27, 2020 14:33

Don't store actions on jobs

cf24011

mrginglymus force-pushed the freestyle branch from 2b3f687 to cf24011 Compare December 27, 2020 14:46

mrginglymus added 2 commits December 27, 2020 15:38

Run GitHubChecksPublisherITest on freestyle and pipeline jobs

52e0a73

Make checkstyle happy

c63bc35

mrginglymus marked this pull request as ready for review December 27, 2020 15:46

mrginglymus requested a review from a team as a code owner December 27, 2020 15:46

XiongKezhi reviewed Dec 27, 2020

View reviewed changes

Test jobs and runs

999eeee

XiongKezhi approved these changes Dec 27, 2020

View reviewed changes

uhafner reviewed Dec 28, 2020

View reviewed changes

Unify GitHubSCMSourceChecksContext constructors

735772d

mrginglymus requested a review from XiongKezhi December 28, 2020 14:02

XiongKezhi approved these changes Dec 28, 2020

View reviewed changes

XiongKezhi merged commit ee26572 into jenkinsci:master Dec 29, 2020

XiongKezhi added the bug Something isn't working label Dec 29, 2020

mrginglymus deleted the freestyle branch December 29, 2020 16:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't store actions on jobs #99

Don't store actions on jobs #99

mrginglymus commented Dec 27, 2020

codecov bot commented Dec 27, 2020 •

edited

Loading

XiongKezhi Dec 27, 2020

mrginglymus Dec 27, 2020

XiongKezhi Dec 27, 2020 •

edited

Loading

mrginglymus Dec 27, 2020

uhafner Dec 27, 2020 •

edited

Loading

mrginglymus Dec 28, 2020

mrginglymus Dec 28, 2020

XiongKezhi Dec 28, 2020 •

edited

Loading

XiongKezhi Dec 28, 2020

mrginglymus Dec 28, 2020

mrginglymus commented Dec 27, 2020

XiongKezhi commented Dec 27, 2020

timja commented Dec 28, 2020

mrginglymus commented Dec 28, 2020 •

edited

Loading

uhafner Dec 27, 2020

mrginglymus Dec 28, 2020

uhafner Dec 28, 2020

XiongKezhi left a comment

Don't store actions on jobs #99

Don't store actions on jobs #99

Conversation

mrginglymus commented Dec 27, 2020

codecov bot commented Dec 27, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XiongKezhi Dec 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uhafner Dec 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XiongKezhi Dec 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrginglymus commented Dec 27, 2020

XiongKezhi commented Dec 27, 2020

timja commented Dec 28, 2020

mrginglymus commented Dec 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XiongKezhi left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 27, 2020 •

edited

Loading

XiongKezhi Dec 27, 2020 •

edited

Loading

uhafner Dec 27, 2020 •

edited

Loading

XiongKezhi Dec 28, 2020 •

edited

Loading

mrginglymus commented Dec 28, 2020 •

edited

Loading