[JENKINS-51627] the code was just plain wrong and ugly :) #1745

jtnord · 2018-06-01T13:33:07Z

Tidy up the code that prevents multiple "try blue" actions from showing.
The code attempted to make sure that only one "Try BlueOcean" action was shown butno one looked at the reason why multiple where shown.
The reason that multiple actions where shown was a bug in core that affected Queue.Items and their subclasses. Given that almost all Queue.Items have no page of their own there is little point in creating an Action for them (and this solves the fact that they get persisted and then shown for a AbstractBuild.

In the case they are already persisted we just replace the action with an invisibleAction such that the only rendered "Try Blue" will be the one created afresh from the TransientActionFactory

NOtes to tester (this is untested). TO verify run the existing code and create a freestyle job.
build it multiple times.
stop jenkins and validate in the build.xml you have multiple blueActions defined.
start jenkins and update the code to the version contained here.
restart jenkins and do a few more builds.
check the old builds just have one BLue Action shown. check the new builds have one blue action shown.

Submitter checklist

Link to JIRA ticket in description, if appropriate.
Change is code complete and matches issue description
Appropriate unit or acceptance tests or explanation to why this change has no tests
Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.

Reviewer checklist

Run the changes and verified the change matches the issue description
Reviewed the code
Verified that the appropriate tests have been written or valid explanation given

The code attempted to make sure that only one "Try BlueOcean" action was shown butno one looked at the reason why multiple where shown. The reason that multiple actions where shown was a bug in core that affected Queue.Items and their subclasses. Given that almost all Queue.Items have no page of their own there is little point in creating an Action for them (and this solves the fact that they get persisted and then shown for a AbstractBuild. In the case they are already persisted we just replace the action with an invisibleAction such that the only rendered "Try Blue" will be the one created afresh from the TransientActionFactory

vivek · 2018-06-04T15:31:14Z

As noted in this PR, its untested so please do not merge. We need to first test it works for all 'Open BlueOcean' links.

jtnord · 2018-06-04T15:37:19Z

@vivek to be clear, the same approach has been used internally, so it is not completely untested (it is just not tested for BlueOcean)

vivek

@jtnord Thanks for the PR, before we merge, it needs testing to ensure no regressions and it does what its supposed to do :)

vivek · 2018-06-04T15:51:31Z

the same approach has been used internally, so it is not completely untested (it is just not tested for BlueOcean)

@jtnord Good to know, guess work in progress label is not needed in that case. Lets merge it after more testing.

vivek · 2018-06-04T16:35:17Z

blueocean-rest-impl/src/main/java/io/jenkins/blueocean/service/embedded/BlueOceanUrlAction.java

+        return DoNotShowPersistedBlueOceanUrlActions.INSTANCE;
+    }
+
+    private static final class DoNotShowPersistedBlueOceanUrlActions extends InvisibleAction {


@jtnord Build is failing as jacoco wants 100% class coverage.

[WARNING] Rule violated for bundle blueocean-rest-impl: classes missed count is 1.00, but expected maximum is 0.00

vivek

Code looks good. Lets get it properly tested and then we can merge it.

vivek · 2018-06-04T17:25:21Z

...an-rest-impl/src/test/java/io/jenkins/blueocean/service/embedded/BlueOceanUrlActionTest.java

+
+    @Test
+    public void testMigration() {
+        BlueOceanUrlObject mock = mock(BlueOceanUrlObject.class);


Awesome, thanks @jtnord .

kshultzCB · 2018-06-06T20:29:55Z

@jtnord , I'm working on the testing following your steps, but don't see any blueActions defined in any of the build.xml files. Here's a sample of one of them:

<?xml version='1.1' encoding='UTF-8'?>
<build>
  <actions>
    <hudson.model.CauseAction>
      <causeBag class="linked-hash-map">
        <entry>
          <hudson.model.Cause_-UserIdCause>
            <userId>admin</userId>
          </hudson.model.Cause_-UserIdCause>
          <int>1</int>
        </entry>
      </causeBag>
    </hudson.model.CauseAction>
  </actions>
  <queueId>11</queueId>
  <timestamp>1528316087968</timestamp>
  <startTime>1528316087987</startTime>
  <result>SUCCESS</result>
  <duration>1135</duration>
  <charset>UTF-8</charset>
  <keepLog>false</keepLog>
  <builtOn></builtOn>
  <workspace>/home/kshultz/GitHub/blueocean-plugin/blueocean/work/workspace/UX-725</workspace>
  <hudsonVersion>2.107.2</hudsonVersion>
  <scm class="hudson.scm.NullChangeLogParser"/>
  <culprits class="com.google.common.collect.EmptyImmutableSortedSet"/>
</build>

I'm running the "before" scenario off of blueocean/master, which works out to core 2.107.2:

jtnord · 2018-06-06T21:19:58Z

try doing the builds in classic and see if there is a difference....
the ugly remove in a try catch could be 'fixing' things under the hood but not in the right way.

kshultzCB · 2018-06-07T01:39:58Z

I tried it both ways, from BO and Classic. Neither produced the blueActions. In the morning I'll give it a go with two new jobs - one purely run from classic, the other from BO.

This Freestyle job is nothing more than a shell step. It's config.xml is below. Any reason to suspect a different sort of job would help? I can't think of one, but stuff like that still surprises me occasionally.

<?xml version='1.1' encoding='UTF-8'?>
<project>
  <description></description>
  <keepDependencies>false</keepDependencies>
  <properties/>
  <scm class="hudson.scm.NullSCM"/>
  <canRoam>true</canRoam>
  <disabled>false</disabled>
  <blockBuildWhenDownstreamBuilding>false</blockBuildWhenDownstreamBuilding>
  <blockBuildWhenUpstreamBuilding>false</blockBuildWhenUpstreamBuilding>
  <triggers/>
  <concurrentBuild>false</concurrentBuild>
  <builders>
    <hudson.tasks.Shell>
      <command>java -jar /home/kshultz/bin/jenkins-cli.jar -auth admin:admin -s http://127.0.0.1:8080/jenkins list-jobs</command>
    </hudson.tasks.Shell>
  </builders>
  <publishers/>
  <buildWrappers/>
</project>

kshultzCB · 2018-06-07T13:43:43Z

Not able to recreate using classic-only. Here's the project itself, just a simple shell step in a freestyle job:

<?xml version='1.1' encoding='UTF-8'?>
<project>
  <description></description>
  <keepDependencies>false</keepDependencies>
  <properties/>
  <scm class="hudson.scm.NullSCM"/>
  <canRoam>true</canRoam>
  <disabled>false</disabled>
  <blockBuildWhenDownstreamBuilding>false</blockBuildWhenDownstreamBuilding>
  <blockBuildWhenUpstreamBuilding>false</blockBuildWhenUpstreamBuilding>
  <triggers/>
  <concurrentBuild>false</concurrentBuild>
  <builders>
    <hudson.tasks.Shell>
      <command>netstat -a</command>
    </hudson.tasks.Shell>
  </builders>
  <publishers/>
  <buildWrappers/>
</project>

Here are the ${build-number}/build.xml files

✔ ~/GitHub/blueocean-plugin/blueocean/work/jobs/classic-only/builds [master|✔] 
09:40 $ cat 2/build.xml 
<?xml version='1.1' encoding='UTF-8'?>
<build>
  <actions>
    <hudson.model.CauseAction>
      <causeBag class="linked-hash-map">
        <entry>
          <hudson.model.Cause_-UserIdCause>
            <userId>admin</userId>
          </hudson.model.Cause_-UserIdCause>
          <int>1</int>
        </entry>
      </causeBag>
    </hudson.model.CauseAction>
  </actions>
  <queueId>2</queueId>
  <timestamp>1528378765479</timestamp>
  <startTime>1528378765500</startTime>
  <result>SUCCESS</result>
  <duration>719</duration>
  <charset>UTF-8</charset>
  <keepLog>false</keepLog>
  <builtOn></builtOn>
  <workspace>/home/kshultz/GitHub/blueocean-plugin/blueocean/work/workspace/classic-only</workspace>
  <hudsonVersion>2.107.2</hudsonVersion>
  <scm class="hudson.scm.NullChangeLogParser"/>
  <culprits class="com.google.common.collect.EmptyImmutableSortedSet"/>
</build>✔ ~/GitHub/blueocean-plugin/blueocean/work/jobs/classic-only/builds [master|✔] 
09:40 $ !diff
diff 1/build.xml 2/build.xml 
15,17c15,17
<   <queueId>1</queueId>
<   <timestamp>1528378761761</timestamp>
<   <startTime>1528378761777</startTime>
---
>   <queueId>2</queueId>
>   <timestamp>1528378765479</timestamp>
>   <startTime>1528378765500</startTime>
19c19
<   <duration>1005</duration>
---
>   <duration>719</duration>

kshultzCB · 2018-06-07T14:09:32Z

Same issue with a freestyle job which is only ever run from within Blue Ocean (i.e., create the job from Classic, immediately switch, and hit the "Run" button when it tells you it's never been run before).

scherler · 2018-06-13T20:37:19Z

@jtnord @kshultzCB ping

kshultzCB · 2018-06-15T17:35:26Z

I was never able to recreate the issue as originally described while running from master. So, I switched to James' branch (git fetch jtnord; git checkout jtnord/JENKINS-51627; mvn clean install; mvn -f blueocean/pom.xml hpi:run). Can't recreate it there either.

✔ ~/GitHub/blueocean-plugin/blueocean/work/jobs [:df536055b|✔] 
13:34 $ grep -i -r blueActions . | wc -l
0

vivek · 2018-06-15T18:00:19Z

@kshultzCB Reading James's notes, we should at least see one blueAction, no? Did you try on UI, do you see Open blueocean button - one and only one instance on different classic page, dashboard, job page, specific run page etc.?

kshultzCB · 2018-06-15T18:29:56Z

Running this branch:

The links are present in the UI
I ran through a handful of jobs and checked each link on the left side in turn, for one Freestyle job, and one branch in a MBP job, just to be sure.
I only saw one instance of a Open Blue Ocean link anywhere in the Classic UI.

Running from master:

I was never able to recreate the described issue of multiple Open Blue Ocean links
I've seen that issue in the past, but it's been a while.
I also did not see any instances of the string blueAction in any files, on-disk, in blueocean-plugin/blueocean/work/jobs/*. That's the part I was expecting to see based on James' notes, but didn't.

jtnord · 2018-06-20T16:38:07Z

you may not be able to see any actions persisted as you are removing them. Not all ModelObjects allow removal of actions (hence the ugly catch (Throwable), but regardless I stand by the code is plain wrong and ugly. You may have had to use an older version to get the links persisted (the readResolve will fix this issue if it has actually occured).

Anything that does a

try { 
 // something
} catch (Throwable) {
 // ignore it
}

is bad code.

vivek · 2018-06-22T00:28:30Z

@jtnord So what @kshultzCB described is ok behavior? Karl already confirmed functionally it looks good. I am trying to get sense if its ready to merge.

jtnord · 2018-06-22T11:00:47Z

@vivek yes the behaviour is OK.

vivek · 2018-06-26T14:30:23Z

Got OK from @kshultzCB, its ready to be merged.

olamy self-assigned this Jun 2, 2018

scherler approved these changes Jun 4, 2018

View reviewed changes

vivek added DO NOT MERGE work-in-progress labels Jun 4, 2018

vivek reviewed Jun 4, 2018

View reviewed changes

vivek removed the work-in-progress label Jun 4, 2018

vivek requested changes Jun 4, 2018

View reviewed changes

[JENKINS-51627] add a unit test to check serialisation.

df53605

jtnord force-pushed the JENKINS-51627 branch from 1a4fd64 to df53605 Compare June 4, 2018 17:23

vivek approved these changes Jun 4, 2018

View reviewed changes

vivek merged commit 77960a8 into jenkinsci:master Jun 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-51627] the code was just plain wrong and ugly :) #1745

[JENKINS-51627] the code was just plain wrong and ugly :) #1745

jtnord commented Jun 1, 2018 •

edited

Loading

vivek commented Jun 4, 2018

jtnord commented Jun 4, 2018

vivek left a comment •

edited

Loading

vivek commented Jun 4, 2018

vivek Jun 4, 2018

vivek left a comment

vivek Jun 4, 2018

kshultzCB commented Jun 6, 2018

jtnord commented Jun 6, 2018

kshultzCB commented Jun 7, 2018

kshultzCB commented Jun 7, 2018

kshultzCB commented Jun 7, 2018

scherler commented Jun 13, 2018

kshultzCB commented Jun 15, 2018

vivek commented Jun 15, 2018

kshultzCB commented Jun 15, 2018

jtnord commented Jun 20, 2018

vivek commented Jun 22, 2018

jtnord commented Jun 22, 2018

vivek commented Jun 26, 2018

[JENKINS-51627] the code was just plain wrong and ugly :) #1745

[JENKINS-51627] the code was just plain wrong and ugly :) #1745

Conversation

jtnord commented Jun 1, 2018 • edited Loading

Submitter checklist

Reviewer checklist

vivek commented Jun 4, 2018

jtnord commented Jun 4, 2018

vivek left a comment • edited Loading

Choose a reason for hiding this comment

vivek commented Jun 4, 2018

vivek Jun 4, 2018

Choose a reason for hiding this comment

vivek left a comment

Choose a reason for hiding this comment

vivek Jun 4, 2018

Choose a reason for hiding this comment

kshultzCB commented Jun 6, 2018

jtnord commented Jun 6, 2018

kshultzCB commented Jun 7, 2018

kshultzCB commented Jun 7, 2018

kshultzCB commented Jun 7, 2018

scherler commented Jun 13, 2018

kshultzCB commented Jun 15, 2018

vivek commented Jun 15, 2018

kshultzCB commented Jun 15, 2018

jtnord commented Jun 20, 2018

vivek commented Jun 22, 2018

jtnord commented Jun 22, 2018

vivek commented Jun 26, 2018

jtnord commented Jun 1, 2018 •

edited

Loading

vivek left a comment •

edited

Loading