-
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-59048] Export the spotBlockReservationDurationStr constructor parameter for JCasc #443
Conversation
The Jenkins "Configuration as Code" plugin raises an error while trying to export the `spotBlockReservationDurationStr` attribute from the configuration, as this constructor attribute is not exposed in the `SpotConfiguration` class. Fix: JENKINS-59048
I've requested a review from the jcasc guys because I'm not sure if this is the right fix |
Okay after looking into it my recommendation still stands. ec2-plugin/src/main/resources/hudson/plugins/ec2/SlaveTemplate/config.jelly Lines 61 to 75 in ad224d8
to use a <f:optionalProperty title="${%Spot configuration}" field="spotConfig"/> in the newly created <f:entry title="${%Fallback to on-demand instances}" >
<f:checkbox field="fallbackToOndemand" checked="${instance.spotConfig.fallbackToOndemand}" />
</f:entry>
<f:optionalBlock name="useBidPrice" title="Set bid price" inline="true" checked="${h.defaultToTrue(instance.spotConfig.useBidPrice)}">
<f:validateButton title="${%Check Current Spot Price}" progress="${%Checking...}" method="currentSpotPrice" with="useInstanceProfileForCredentials,credentialsId,region,type,zone,roleArn,roleSessionName,ami" />
<f:entry title="${%Spot Max Bid Price}" field="spotMaxBidPrice">
<f:textbox />
</f:entry>
</f:optionalBlock>
<f:entry title="${%Spot Block Reservation Duration}" field="spotBlockReservationDuration">
<f:number />
</f:entry> |
Thanks @res0nance, @timja and @jetersen for the review and suggestions! I'm still new to Jenkins plugin development, so I'll try to dig a bit more. If I sum up:
|
So, I think I addressed most of the comments:
This changes a few things though:
There's still a test failing in |
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.
One minor nit pick! Otherwise excellent job! 😍
<f:checkbox field="fallbackToOndemand" checked="${instance.spotConfig.fallbackToOndemand}" /> | ||
</f:entry> | ||
|
||
<f:optionalBlock name="useBidPrice" title="Set bid price" inline="true" checked="${h.defaultToTrue(instance.spotConfig.useBidPrice)}"> |
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.
Not sure this optional block makes sense... however I do not use the plugin.
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.
Well, it doesn't really make sense to me, as I explained above:
- Either you selected "I want to request a Spot instance" and you need to specify a price (this is mandatory on the AWS' side)
- Or, you don't want to request Spot instances, in which case the spotConfig block is just not set.
I could make that change here, but I think it goes a bit out of scope of the initial issue, and it will require more change in the code to reflect that new "logic" (it's a bit convoluted at the moment IMO).
So, I'd prefer to stay at the same feature level for that PR :)
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.
Not sure this optional block makes sense...
Actually, why do you think it doesn't make sense?
The failing test is actually on that attribute, so may be I misunderstood your comment :)
JCasC by design does not print default values when exporting. I may be able to look into the test failure tomorrow if needed. |
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.
I haven’t checked the UI of how this looks, but the jcasc side of it looks a lot better, and having the test updates greatly increases the confidence, for now and future changes
RE: making the fields private, that’s a breaking change and not recommended, the usual approach for that is creating a new field and migrating the old data. Not worth it here imo |
These fields are already defaulting to these values Co-Authored-By: Joseph Petersen <josephp90@gmail.com>
The test failure is actually a genuine bug: I tried manually to save a config where "Set bid price" was not checked, and it appears again as "checked" when reloading the page. |
Found the issue ( 🤞 ): I forgot to remove the access to the |
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.
Definitely a step in the right direction, good job @multani 🙇♂️
Extension got lost when it changed directory
Arf, apparently, the "Check Current Spot Price" button doesn't work anymore ... 🤦♂️ |
The button moved along the SpotConfiguration Jelly but not the attached action. I also moved the non-SlaveTemplate related functions used as static methods in CloudHelper, as they are being used in several places.
I don't think the test failure is related to my branch, is it possible to restart the build? |
if you close and reopen the PR it'll trigger it again |
Hi, is there anything else that I must do on this pull request? |
it's up to the maintainers for the plugin to review and merge 😅 I was only pinged to review because of the JCasC side of things. |
@thoulen Thanks a lot for the merge! I wanted to updated JENKINS-59048 to reflect the changes, but I keep getting HTTP 403 errors whatever I'm trying to do (watch the ticket, comment, etc.) 🤷♂️ |
@multani update it on https://issues.jenkins-ci.org, the domain you're on doesn't work |
The Jenkins "Configuration as Code" plugin raises an error while trying
to export the
spotBlockReservationDurationStr
attribute from theconfiguration, as this constructor attribute is not exposed in the
SpotConfiguration
class.Fix: JENKINS-59048