Skip to content

[JENKINS-43786] Adapted the administrative monitor to the new UI definition #70

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.

Sign up for GitHub

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

Merged
oleg-nenashev merged 7 commits into from
Feb 26, 2018
Merged

[JENKINS-43786] Adapted the administrative monitor to the new UI definition #70

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
25bb207
[JENKINS-43786] Adapted the administrative monitor to the new UI defi…
recena Sep 14, 2017
fc05f37
[JENKINS-43786] More compatible version
recena Oct 29, 2017
faf2ae8
[JENKINS-43786] Addressed a @daniel-beck's comment
recena Oct 29, 2017
782cff7
[JENKINS-43786] Updated some details before merging
recena Jan 23, 2018
8e22924
[JENKINS-43786] Checks possible NPE
recena Jan 28, 2018
c28f0fe
Merge branch 'master' of https://github.com/jenkinsci/ssh-slaves-plug…
recena Jan 28, 2018
732ff1d
[JENKINS-43786] - Polish Javadoc to kick-off the build
oleg-nenashev Feb 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion .../hudson/plugins/sshslaves/verifiers/MissingVerificationStrategyAdministrativeMonitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import hudson.plugins.sshslaves.SSHLauncher;
import hudson.slaves.ComputerLauncher;
import hudson.slaves.SlaveComputer;
import hudson.util.VersionNumber;
import jenkins.model.Jenkins;

/**
Expand All @@ -45,7 +46,7 @@ public boolean isActivated() {
for (Computer computer : Jenkins.getActiveInstance().getComputers()) {
if (computer instanceof SlaveComputer) {
ComputerLauncher launcher = ((SlaveComputer) computer).getLauncher();

if (launcher instanceof SSHLauncher && null == ((SSHLauncher) launcher).getSshHostKeyVerificationStrategy()) {
return true;
}
Expand All @@ -55,4 +56,15 @@ public boolean isActivated() {
return false;
}

/**
* This method can be removed when the baseline is updated to 2.88
*
* @return If this version of the plugin is running on a Jenkins version where JENKINS-43786 is included.
*/
public boolean isTheNewDesignAvailable() {

@daniel-beck daniel-beck Oct 29, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Restricted(DoNotUse.class)

@recena recena Oct 29, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daniel-beck Done.

if (Jenkins.getVersion().isNewerThan(new VersionNumber("2.88"))) {

@recena recena Oct 29, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@oleg-nenashev oleg-nenashev Oct 31, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@recena recena Oct 31, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return true;
}
return false;
}
}
18 changes: 14 additions & 4 deletions ...lugins/sshslaves/verifiers/MissingVerificationStrategyAdministrativeMonitor/message.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,18 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
THE SOFTWARE.
-->
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form" xmlns:c="/lib/credentials">
<div class="warning">
<p>SSH Host Key Verifiers are not configured for all SSH slaves on this Jenkins instance. This could leave these slaves open to man-in-the-middle attacks. <a href="${rootURL}/computer/">Update your slave configuration</a> to resolve this.</p>
</div>
<j:jelly xmlns:j="jelly:core">

<j:if test="${!it.isTheNewDesignAvailable}">

@daniel-beck daniel-beck Oct 29, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@recena recena Oct 29, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@oleg-nenashev oleg-nenashev Oct 29, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@recena recena Oct 30, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ndeloof ndeloof Jun 6, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@recena recena Jun 6, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ndeloof A var for what?

@recena recena Jun 6, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ndeloof ndeloof Jun 6, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@recena recena Jun 6, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@recena recena Jun 6, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<div class="warning">
<p>SSH Host Key Verifiers are not configured for all SSH slaves on this Jenkins instance. This could leave these slaves open to man-in-the-middle attacks. <a href="${rootURL}/computer/">Update your slave configuration</a> to resolve this.</p>
</div>
</j:if>

<j:if test="${it.isTheNewDesignAvailable}">
<div class="alert alert-warning">
SSH Host Key Verifiers are not configured for all SSH slaves on this Jenkins instance. This could leave these slaves open to man-in-the-middle attacks. <a href="${rootURL}/computer/">Update your slave configuration</a> to resolve this.
</div>
</j:if>

</j:jelly>