[JENKINS-43786] Adapted the administrative monitor to the new UI definition by recena · Pull Request #70 · jenkinsci/ssh-agents-plugin

recena · 2017-09-14T18:33:36Z

JENKINS-43786

Downstream of jenkinsci/jenkins#2857

Screenshot

@jenkinsci/code-reviewers

…nition

rtyler · 2017-09-14T18:42:59Z

looks pretty 👍

recena · 2017-09-14T18:46:13Z

@rtyler Thanks for your feedback. I've tried to provide an improvement in the UI by using a progressive enhancement approach. The previous one works and the maintainers will be able to adapt their plugins step by step.

daniel-beck · 2017-09-14T18:56:32Z

How does it look on Jenkins before the core PR?

recena · 2017-09-15T08:51:18Z

@daniel-beck I can prepare a screenshot but you can see an example jenkinsci/jenkins#2857 (comment).

daniel-beck · 2017-09-15T09:44:54Z

@recena To clarify, plugins that include a change like this one (i.e. the class attribute) will look exactly like admin monitors in core in the respective core releases? If old core, looks like old admin monitors; and if new core (once the PR is merged), it'll look like new core monitors?

recena · 2017-09-19T18:41:29Z

@daniel-beck This is a screenshot by using the changes on this PR and Jenkins LTS 2.60.1.

Although we could find some bad results.

oleg-nenashev

alert and alert-warning CSS classes have been introduced in Jenkins 1.633. jenkinsci/jenkins@83a8d54

The plugin baseline is 1.625.x now. It is fine to bump the baseline IMHO, but it has to be done in order to make this PR acceptable.

🐛

recena · 2017-09-21T09:04:38Z

@oleg-nenashev You can close this PR if you don't want to use this new design provided by the core. Nothing has been removed, hence, you can continue using the previus implementation.

Maintainers can decide when starting to use this new UI design.

recena · 2017-09-21T09:08:01Z

@oleg-nenashev According to the stats, I don't see your concern like something critical 😄

oleg-nenashev · 2017-09-21T09:10:56Z

@recena I do not understand how your response about "don't want" is related to my review feedback, please read it again.

I just said that the PR is not ready, because it uses styles not available in the baseline.
I explicitly said the baseline upgrade is fine

recena · 2017-09-21T09:38:10Z

@oleg-nenashev You agree, I was not clear with my explanation. What I wanted to say is the maintainer decides how and when he wants to adapt his plugin.

Sure, I'm going to bump the baseline to 1.633.

oleg-nenashev · 2017-09-21T09:41:42Z

Sure, I'm going to bump the baseline to 1.633.

Please use 1.642.3 LTS then. It's better to build against LTS releases, and nobody cares much about such old weeklys for sure

recena · 2017-10-29T15:12:47Z

...ava/hudson/plugins/sshslaves/verifiers/MissingVerificationStrategyAdministrativeMonitor.java

+     * @return If this version of the plugin is running on a Jenkins version where JENKINS-43786 is included.
+     */
+    public boolean isTheNewDesignAvailable() {
+        if (Jenkins.getVersion().isNewerThan(new VersionNumber("2.88"))) {


2.88 is only an example. it depends when jenkinsci/jenkins#2857 is merged and released.

recena · 2017-10-29T15:14:52Z

@oleg-nenashev @daniel-beck @jglick

You can see one way on how the plugins could use jenkinsci/jenkins#2857

daniel-beck · 2017-10-29T20:25:26Z

...ava/hudson/plugins/sshslaves/verifiers/MissingVerificationStrategyAdministrativeMonitor.java

+     *
+     * @return If this version of the plugin is running on a Jenkins version where JENKINS-43786 is included.
+     */
+    public boolean isTheNewDesignAvailable() {


@Restricted(DoNotUse.class)

@daniel-beck Done.

daniel-beck · 2017-10-29T20:25:55Z

...n/plugins/sshslaves/verifiers/MissingVerificationStrategyAdministrativeMonitor/message.jelly

-    </div>
+<j:jelly xmlns:j="jelly:core">
+
+<j:if test="${!it.isTheNewDesignAvailable}">


Should just be made into a Groovy view so there's no change to plugin classes needed.

@daniel-beck Agree although I'm not fan of Groovy for that purpose. I wanted to offer to two different examples. This one, and this jenkinsci/github-plugin#179, where the maintainers are already using Groovy.

I am fine with a non-Groovy implementation. As we discussed with @recena in the chat, we rather need a macro or tag, which would check the Jenkins core version

@oleg-nenashev Yes, a Jelly tag would be helpful. I'll find some of time for it.

could use j:choose + j:when + j:otherwise
And should use a var to avoid code duplication

@ndeloof A var for what?

Remember, we are considering two cases just temporarily. When the baseline is updated, we don't need it.

a var to define the css class to be used.

<j:choose> <j:when test="${it.isTheNewDesignAvailable}"> <j:set var="css" value="warning"> </j:when> <j:otherwise> <j:set var="css" value="alert alert-warning"> </j:otherwise> </j:choose> <div class="${css}"> ...

makes me wonder, but it seems one could even use this simpler form:

<div class="${it.isTheNewDesignAvailable() ? 'alert alert-warning' : 'warning'}"> ...

by the way, using isNewDesignAvailable() as method name would allow to use property style notation ${it.isNewDesignAvailable} (adding "The" sounds weird to me).

I'm pretty sure this has been intensively investigated, but can't the CSS just target "warning" class efficiently enough within an administrative monitor context so all this isn't required ? I would understand this is required it he DOM structure was different, but here this sounds to only be a question of CSS alias.

It seems to be you are not taking into account the paragraph <p>, needed in the first case.

By the way, as you can read in the article, this is temporal. It could be removed when the baseline is updated.

And by the way, this is the simplest case where the content is more or less similar (but not identical). However, you will find more complex cases here jenkinsci/jenkins#2857

I don't see the value of using a var.

lanwen · 2017-10-30T09:42:57Z

Is it dismissable?

daniel-beck · 2017-10-30T09:48:03Z

@lanwen No button for this one (even before), but there's an Administrative Monitors section on Configure System to disable or enable them individually.

recena · 2017-10-30T11:39:09Z

@lanwen As @daniel-beck has mentioned, I am not changing the functionality, only the appearance.

recena · 2017-10-30T20:46:01Z

@oleg-nenashev Could you update your review, please?

oleg-nenashev

🐝 works for me, but it needs the Jenkins core release first

oleg-nenashev · 2017-10-31T12:50:19Z

...ava/hudson/plugins/sshslaves/verifiers/MissingVerificationStrategyAdministrativeMonitor.java

+     */
+    @Restricted(DoNotUse.class)
+    public boolean isTheNewDesignAvailable() {
+        if (Jenkins.getVersion().isNewerThan(new VersionNumber("2.88"))) {


This version needs to be updated to the actual one when upstream changes are integrated

Sure, for that reason this PR is downstream of jenkinsci/jenkins#2857

oleg-nenashev · 2018-01-22T18:59:34Z

@recena 2.103 has been released. Would you be able to update the PR so that we could ship it?

oleg-nenashev

🐝

recena · 2018-01-23T10:33:05Z

The tests are passing locally:

-------------------------------------------------------
 T E S T S
-------------------------------------------------------
Running hudson.plugins.sshslaves.verifiers.ManuallyProvidedKeyVerificationStrategyTest
Running hudson.plugins.sshslaves.SSHLauncherTest
Running hudson.plugins.sshslaves.verifiers.TrileadVersionSupportManagerTest
Running hudson.plugins.sshslaves.verifiers.TrustHostKeyActionTest
Running hudson.plugins.sshslaves.verifiers.VerificationStrategyConfigurationTest
Running InjectedTest
Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.228 sec - in hudson.plugins.sshslaves.verifiers.TrileadVersionSupportManagerTest
Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.585 sec - in hudson.plugins.sshslaves.verifiers.ManuallyProvidedKeyVerificationStrategyTest
Tests run: 32, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 31.557 sec - in InjectedTest
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 42.994 sec - in hudson.plugins.sshslaves.verifiers.TrustHostKeyActionTest
Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 45.932 sec - in hudson.plugins.sshslaves.verifiers.VerificationStrategyConfigurationTest
Tests run: 12, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 57.146 sec - in hudson.plugins.sshslaves.SSHLauncherTest

Results :

Tests run: 55, Failures: 0, Errors: 0, Skipped: 1

it seems something related with the CI env.

oleg-nenashev

The build failure is real 🐛

10:28:47 [linux-2.73.1] [INFO] Total bugs: 1
10:28:47 [linux-2.73.1] [INFO] Possible null pointer dereference in hudson.plugins.sshslaves.verifiers.MissingVerificationStrategyAdministrativeMonitor.isTheNewDesignAvailable() due to return value of called method [hudson.plugins.sshslaves.verifiers.MissingVerificationStrategyAdministrativeMonitor, hudson.plugins.sshslaves.verifiers.MissingVerificationStrategyAdministrativeMonitor] Dereferenced at MissingVerificationStrategyAdministrativeMonitor.java:[line 68]Known null at MissingVerificationStrategyAdministrativeMonitor.java:[line 68] NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE

oleg-nenashev · 2018-01-26T17:48:48Z

Releasing new version without it

recena · 2018-01-28T09:40:49Z

@oleg-nenashev I don't know why I cannot see this error. I've checked the possible null.

… into JENKINS-43786

oleg-nenashev · 2018-02-16T10:05:37Z

Restarted the CI build

[JENKINS-43786] Adapted the administrative monitor to the new UI defi…

25bb207

…nition

recena mentioned this pull request Sep 14, 2017

[JENKINS-43786] Overhaul of Manage Jenkins page jenkinsci/jenkins#2857

Merged

3 tasks

andresrc approved these changes Sep 15, 2017

View reviewed changes

oleg-nenashev requested changes Sep 20, 2017

View reviewed changes

recena force-pushed the JENKINS-43786 branch from 4568365 to 1f810dd Compare October 29, 2017 15:07

[JENKINS-43786] More compatible version

fc05f37

recena force-pushed the JENKINS-43786 branch from 1f810dd to fc05f37 Compare October 29, 2017 15:10

recena commented Oct 29, 2017

View reviewed changes

recena mentioned this pull request Oct 29, 2017

[JENKINS-43786] Adapted the administrative monitor to the new UI definition jenkinsci/github-plugin#177

Merged

daniel-beck reviewed Oct 29, 2017

View reviewed changes

[JENKINS-43786] Addressed a @daniel-beck's comment

faf2ae8

ikedam mentioned this pull request Oct 29, 2017

[JENKINS-43786] More compatible adaptation jenkinsci/github-plugin#179

Open

oleg-nenashev approved these changes Oct 31, 2017

View reviewed changes

oleg-nenashev added the work-in-progress label Dec 12, 2017

[JENKINS-43786] Updated some details before merging

782cff7

oleg-nenashev removed the work-in-progress label Jan 23, 2018

oleg-nenashev approved these changes Jan 23, 2018

View reviewed changes

oleg-nenashev requested changes Jan 24, 2018

View reviewed changes

recena and others added 3 commits January 28, 2018 10:43

[JENKINS-43786] Checks possible NPE

8e22924

Merge branch 'master' of https://github.com/jenkinsci/ssh-slaves-plugin…

c28f0fe

… into JENKINS-43786

[JENKINS-43786] - Polish Javadoc to kick-off the build

732ff1d

oleg-nenashev approved these changes Feb 26, 2018

View reviewed changes

oleg-nenashev merged commit 9a017f4 into jenkinsci:master Feb 26, 2018

basil mentioned this pull request Jul 20, 2019

Remove dead code for no-longer-supported baselines #137

Merged

Uh oh!

Comments

Conversation

recena commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshot

Uh oh!

rtyler commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

recena commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daniel-beck commented Sep 14, 2017

Uh oh!

recena commented Sep 15, 2017

Uh oh!

daniel-beck commented Sep 15, 2017

Uh oh!

recena commented Sep 19, 2017

Uh oh!

oleg-nenashev left a comment

Choose a reason for hiding this comment

Uh oh!

recena commented Sep 21, 2017

Uh oh!

recena commented Sep 21, 2017

Uh oh!

oleg-nenashev commented Sep 21, 2017

Uh oh!

recena commented Sep 21, 2017

Uh oh!

oleg-nenashev commented Sep 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

recena commented Oct 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

recena Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lanwen commented Oct 30, 2017

Uh oh!

daniel-beck commented Oct 30, 2017

Uh oh!

recena commented Oct 30, 2017

Uh oh!

recena commented Oct 30, 2017

Uh oh!

oleg-nenashev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleg-nenashev commented Jan 22, 2018

recena commented Sep 14, 2017 •

edited

Loading

rtyler commented Sep 14, 2017 •

edited

Loading

recena commented Sep 14, 2017 •

edited

Loading

recena Jun 6, 2018 •

edited

Loading