-
Notifications
You must be signed in to change notification settings - Fork 690
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.
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
[JENKINS-36544] Fix Node Properties on Jenkins 2.205+ #440
Conversation
This is a follow-up fix for PR jenkinsci#382. In 2.204, the "Configure Clouds" screen was on the main /configure system configuration page. As a result, `${it}` referred to the global system object, aka the Hudson instance. In 2.205, PR jenkinsci/jenkins#4339 merged, which moves the Configure Clouds section to a new page: /configureClouds. As a result of that change, `${it}` no longer refers to the Hudson instance, but to a new GlobalCloudConfiguration object. That object does not have the `getNodePropertyDescriptors()` method, so it is not able to populate the "Node Properties" block. Switching that to `${app}` gives it a correct reference to the Hudson object, and that allows it to show the Node Properties block on all Jenkins versions, both before and after v2.205.
cc @res0nance @ro-kr @mildsunrise per original PR participants. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you attach some screen shoots of testing this PR with the latest weekly and any version previous to 2.205? According to https://wiki.jenkins.io/display/JENKINS/Basic+guide+to+Jelly+usage+in+Jenkins (section Other predefined objects):
Depending on the page being rendered, other objects besides it may be predefined:
- app - the instance of Jenkins (or Hudson)
- instance - an object currently being configured, within a section of a configure page such as a BuildStep; null if this is a newly added instance rather than reconfiguring
- descriptor - the Descriptor object (see below) corresponding to the class of instance
- h - an instance of hudson.Functions, with various useful functions
so my understanding is that ${app.getNodePropertyDescriptors()}
should call the Jenkins.getNodePropertyDescriptors()
method, which I cannot find in https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/jenkins/model/Jenkins.java
I'm requesting changes because I'm not totally sure this change would work because of what I've explained above. Obvious I might be wrong, but I'd like all of us to be on the same page. Thanks in advance!
@fcojfernandez Of course! I tried to explain why it works, but much of the information is kind of spread out across here, the linked PR #382, the Jira ticket JENKINS-36544, as well as in the commit message. My mistake for not adding more screenshots. The original PR was where I was trying to debug why it doesn't work for me. That PR conversation starts here: #382 (comment). And specifically where I finally tracked it down to the problem in this comment: #382 (comment) 2.204 and earlier, the Configure Clouds section was on the main system configuration page ($JENKINS/configure), and on that page, The reason you don't see the method on the That means the original PR added the In all the cases below, my test methodology is:
Using the jenkins.version that's defined in this project's pom.xml file (2.150.1), which is before 2.204 and has the Configure Clouds section on the main system configuration screen, this screenshot shows the Node Properties block working correctly with my change (note: it also worked without my change): Updating to version 2.205, where the Configure Clouds section moved to a new page. This fails with current master (screenshot taken from the attachment in Jira, was also shown in the original PR #382 (comment)): but works correctly with my change: The latest version I can find is Jenkins 2.225, which also works: I hope that helps explain what's going on. Let me know if there's anything else I can help clarify. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for the explanation!
True! That's an issue, we should call the method I added to |
@mildsunrise if that was your intent, then it would need to be moved into the Descriptor instead, and called with But it's worth noting that the original change already had this behavior. So I really think it's a different fix, and shouldn't be combined into one. But I'll make the change if we just want it fixed. |
Current master is calling `${it.getNodePropertyDescriptors()}`, but on Jenkins versions <= 2.204, that is calling the method on the global Jenkins node (via Node class). It should instead be using the version of the method that was added in SlaveTemplate, and to do that from a Jelly script, it needs to be moved into the SlaveTemplate.DescriptorImpl class.
Fully agree, that's a different fix, and yes it should be put in the descriptor (apologies, I was confused about Jenkins when I wrote the original PR). I was going to open a fix, but @troymohl did while I was writing the comment, thanks guys 😊💙 |
I went ahead and made the change here, because #438 alone is not enough to fix it. And technically this still fixes the issue of it not working on 2.205+ :) I tested the change, and it still works properly with the move to Descriptor. |
This is a follow-up fix for PR #382.
In 2.204, the "Configure Clouds" screen was on the main /configure system
configuration page. As a result,
${it}
referred to the global systemobject, aka the Hudson instance.
In 2.205, PR jenkinsci/jenkins#4339 merged, which moves the Configure Clouds
section to a new page: /configureClouds. As a result of that change,
${it}
no longer refers to the Hudson instance, but to a new
GlobalCloudConfiguration object. That object does not have the
getNodePropertyDescriptors()
method, so it is not able to populate the"Node Properties" block.
Switching that to
${app}
gives it a correct reference to the Hudson object,and that allows it to show the Node Properties block on all Jenkins versions,
both before and after v2.205.