[JENKINS-25106] Support minimumNoInstances #401
Conversation
Fixes #JENKINS-25106
Co-Authored-By: Francisco Fernández <31063239+fcojfernandez@users.noreply.github.com>
| @@ -126,6 +128,8 @@ | |||
|
|
|||
| public int instanceCap; | |||
|
|
|||
| public int minimumNumberOfInstances; | |||
There was a problem hiding this comment.
No need to add more public fields here. That's not a great practice in Java.
There was a problem hiding this comment.
Indeed. I probably just followed the existing pattern there.
There was a problem hiding this comment.
Commited a fix
|
|
||
| import java.util.List; | ||
|
|
||
| public abstract class EC2SlaveConfig { |
There was a problem hiding this comment.
I'm just think out loud here, but this config with builder class pattern being developed here seems like a nice candidate for improving Jenkins' databinding feature to natively support this type of pattern.
There was a problem hiding this comment.
I'm generally of fan of builder patterns as it shows the user of the class a very verbose representation of what's used in constructing the class. withSomeProperty() is a lot more easier to understand at a quick glance than Constructor parameter #5 :)
That being said, I don't have that much experience with Jenkins' databinding, or Jenkins at all (other than using it) so I don't feel qualified to judge if this pattern works everywhere else :)
There was a problem hiding this comment.
Yeah, I don't, either, though I do have extensive experience in the equivalent functionality in Log4j2, so it's an active interest of mine in general. :)
The gist of my idea at least would be using @DataBoundSetter on the with or set methods and a build() method (or a new annotation), though I'm not sure how that would fit in with Stapler and Jenkins APIs. Hopefully I'll remember to revisit this after figuring that out.
There was a problem hiding this comment.
Haha yeah there's always the issue of remembering stuff.
|
Do you think the new slave-related classes can use the updated term "agent"? We could refactor the old slave classes to use agent in a separate PR, but it'd be easier if we didn't introduce new code with the old name since it's a pain to change when people are already depending on the API. |
Co-Authored-By: Matt Sicker <boards@gmail.com>
Co-Authored-By: Matt Sicker <boards@gmail.com>
Sure! Just |
|
At some point, Jenkins changed the name from "slave" to "agent" officially, though fully migrating everything to the new name would break various existing APIs, so it's mostly been done through the UI and documentation. New code has been encouraged to use the new name. Someone went through the trouble of renaming |
| return instance; | ||
| } | ||
|
|
||
| EC2OndemandSlave createOnDemandSlave(EC2AgentConfig.OnDemand config) throws Descriptor.FormException, IOException; |
There was a problem hiding this comment.
You can also rename the newly added methods.
| } else { | ||
| mock = Mockito.mock(AmazonEC2.class); | ||
| mock = Mockito.mock(AmazonEC2Client.class); |
There was a problem hiding this comment.
Nit: You can use static imports for Mockito methods.
There was a problem hiding this comment.
Added static import now, but I actually prefer not to have them. With Mockito.mock it makes it obvious that it's Mockito and not e.g. PowerMock.
Similarly to assertEquals; are we talking AssertJ, Junit, something else? I just prefer the explicitness :)
Thank you! :) |
|
Thanks :) Let's see if CI ever lets it through though :) |
I'm attempting to open an entirely new PR instead of #343 to see if my weird issues goes away.