Add validations and dynamic default for cloud name (for UI users only) #392
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
jenkinsci#382 Co-authored-by: naraharip2017
pdk27
changed the title
cjerad
reviewed
Aug 7, 2023
Comment on lines
6
to
21
| public class CloudUtil { | ||
|
|
||
| public static Boolean isCloudNameUnique(final String name) { | ||
| return !Jenkins.get().clouds.stream().anyMatch(c -> c.name.equals(name)); | ||
| } | ||
|
|
||
| public static String getUniqDefaultCloudName(final List<String> existingCloudNames, final String defaultCloudName) { | ||
| String uniqName = defaultCloudName; | ||
| int suffix = 1; | ||
| while (existingCloudNames.contains(uniqName)) { | ||
| uniqName = defaultCloudName + "-" + suffix++; | ||
| } | ||
|
|
||
| return uniqName; | ||
| } | ||
| } |
There was a problem hiding this comment.
- It is generally discouraged for class names to include 'Util'. What about
CloudNames(since all the methods deal with the names of Cloud objects)? Then the methods becomeisUniqueandgenerateUnique. - In
getUniqDefaultCloudName, sinceisCloudNameUniquepulls names from theJenkinsinstance it seems that should be an option too. Could refactor into two methodsgenerateUnique(baseName)andgenerateUniqueAmongst(cloudNames, baseName). ThegenerateUniquemethod can then consolidate the logic that is currently in theEC2CloudFleet.getUnqiueCloudNamemethod.
There was a problem hiding this comment.
👍
- I was going for a
helperclass for some future refactoring needs. But,CloudNamesis cleaner. Made the change. - refactored
generateUniquetoo.
There was a problem hiding this comment.
Was this file added by accident? It doesn't appear to be referenced anywhere.
pdk27
commented
Aug 7, 2023
| value = "EC2FleetCloud/name-required-configuration-as-code.yml", | ||
| expected = ConfiguratorException.class, | ||
| message = "error configuring 'jenkins' with class io.jenkins.plugins.casc.core.JenkinsConfigurator configurator") | ||
| public void configurationWithNullName_shouldFail() { |
There was a problem hiding this comment.
name-required-configuration-as-code.yml is used here.
cjerad
approved these changes
Aug 9, 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.
Issue: #387
Follow-up of #390
Changes:
adding cloud name validation to maintain uniqueness
Before, duplicates were allowed. Now, disallowing duplicates for default name suggestions and user provided names as cloud name is used for tracking agents to (volatile) cloud objects and it's name should really be unique. See this for context.
add dynamic default for cloud name
Replacing a static default
FleetCloudwith a dynamic default to avoid duplicates. A number suffix will be added likeFleetCloud-1ifFleetCloudexists.Testing: