Update constructor to follow CasC best practices #406
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pdk27
requested changes
Sep 12, 2023
vineeth-bandi
force-pushed
the
constructor-best-practices
branch
5 times, most recently
from
b4d73ca to
fa8ec22
Compare
vineeth-bandi
force-pushed
the
constructor-best-practices
branch
from
fa8ec22 to
1e0efaa
Compare
vineeth-bandi
force-pushed
the
constructor-best-practices
branch
2 times, most recently
from
627e2a4 to
9ad8ead
Compare
pdk27
requested changes
Sep 20, 2023
|
|
||
| static final int DEFAULT_IDLE_MINUTES = 0; | ||
|
|
||
| static final int DEFAULT_MIN_SIZE = 1; |
There was a problem hiding this comment.
This will always leave 1 instance running unless maxTotalUses is set and met i.e. customer might pay for an idle instance due to this default. Was there a reason to change the default from 0 to 1?
There was a problem hiding this comment.
Per our offline sync, I understand jelly has a defualt of 1. Can we just make the default 0 ? 🤔
There was a problem hiding this comment.
Yup, I'll also change the Jelly to reflect this default. I think it makes more sense to not have idle instances.
| @@ -0,0 +1,33 @@ | |||
| package com.amazon.jenkins.ec2fleet; | |||
|
|
|||
| class CloudConstants { | |||
|
|
||
| @DataBoundSetter | ||
| public void setIdleMinutes(Integer idleMinutes) { | ||
| this.idleMinutes = (idleMinutes != null) ? idleMinutes : 0; |
There was a problem hiding this comment.
since the default is already set for this.idleMinutes
if (idleMinutes != null) {
this.idleMinutes = idleMInutes;
}
| @DataBoundSetter | ||
| public void setMaxSize(int maxSize) { | ||
| if (maxSize < minSize) { | ||
| int newMaxSize = minSize > 0 ? minSize : 1; |
There was a problem hiding this comment.
int newMaxSize = Math.max(1, minSize);?
| public int getMaxTotalUses() { | ||
| return maxTotalUses; | ||
| } | ||
|
|
||
| @DataBoundSetter | ||
| public void setMaxTotalUses(String maxTotalUses) { | ||
| this.maxTotalUses = StringUtils.isBlank(maxTotalUses) ? DEFAULT_MAX_TOTAL_USES : Integer.parseInt(maxTotalUses); |
There was a problem hiding this comment.
Since the default is already set.
if (StringUtils.isNotBlank(maxtotalUses)) {
this.maxTotalUses = Integer.parseInt(maxTotalUses);
}
94b9d28 to
54c9ce4
Compare
pdk27
approved these changes
Sep 21, 2023
…ples, and include EC2FleetLabelCloud examples
vineeth-bandi
force-pushed
the
constructor-best-practices
branch
from
54c9ce4 to
273a031
Compare
pdk27
reviewed
Sep 21, 2023
pdk27
approved these changes
Sep 21, 2023
pdk27
added
bug
documentation
developer
vineeth-bandi
added a commit
that referenced
this pull request
Oct 31, 2023
This reverts commit ae77bfa.
vineeth-bandi
added a commit
that referenced
this pull request
Oct 31, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Created a new constructor and getter and setter methods for non-required values as defined here. This is based on best practices for the CasC plugin as defined here. Added sanity check to ensure
maxSize >= minSize. Created fileCloudConstants.javato define default values forEC2FleetCloudandEC2FleetLabelCloud.Updated Configuration-as-Code docs to reflect new default values, define min and max config examples, and fix typos and incorrect defaults in previous values.
Testing done
Defining new Clouds in UI and CasC is identical to previous behavior. This has no new tests added because the new functionality is covered by existing tests. Tested in UI and CasC that
maxSizewhen set to be lower thanminSizeupdates tominSizewith a warning.Updated CasC unit tests to handle new defaults. New CasC unit test that checks backwards compatibility with deprecated field
credentialsId.Submitter checklist