[jclouds/jclouds] Adds new more relaxed validator for Azure entities (#1213)

[Daniel Estévez <no...@github.com>]

You can view, comment on, or merge this pull request online at:

  https://github.com/jclouds/jclouds/pull/1213

-- Commit Summary --

  * Adds new more relaxed validator for Azure entities

-- File Changes --

    A providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/config/AzureNameValidator.java (61)
    M providers/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/config/AzureComputeHttpApiModule.java (5)

-- Patch Links --

https://github.com/jclouds/jclouds/pull/1213.patch
https://github.com/jclouds/jclouds/pull/1213.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1213

Re: [jclouds/jclouds] Adds new more relaxed validator for Azure entities (#1213)

[Ignasi Barrera <no...@github.com>]

Pushed to master and 2.1.x. Thanks!

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1213#issuecomment-391969708

Re: [jclouds/jclouds] Adds new more relaxed validator for Azure entities (#1213)

[Andrew Phillips <no...@github.com>]

demobox commented on this pull request.



> +
+import org.jclouds.predicates.Validator;
+
+import com.google.common.base.CharMatcher;
+import com.google.inject.Singleton;
+
+/**
+ * Validates name for azure entities
+ * https://docs.microsoft.com/en-us/azure/architecture/best-practices/naming-conventions
+ *
+ * @see org.jclouds.predicates.Validator
+ */
+@Singleton
+public class AzureNameValidator extends Validator<String> {
+   private final int min = 2;
+   private final int max = 63;

[minor] Rename to make the purpose clearer, e.g. `minLength` and `maxLength`. Could these be static variables?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1213#pullrequestreview-123563318

Re: [jclouds/jclouds] Adds new more relaxed validator for Azure entities (#1213)

[Andrew Phillips <no...@github.com>]

demobox commented on this pull request.



> + */
+@Singleton
+public class AzureNameValidator extends Validator<String> {
+   private final int min = 2;
+   private final int max = 63;
+
+   public void validate(String name) {
+
+      if (name == null || name.length() < min || name.length() > max)
+         throw exception(name, "Can't be null or empty. Length must be " + min + " to " + max + " symbols.");
+      if (CharMatcher.JAVA_LETTER_OR_DIGIT.indexIn(name) != 0)
+         throw exception(name, "Should start with letter/number");
+
+      CharMatcher range = getAcceptableRange();
+      if (!range.matchesAllOf(name))
+         throw exception(name, "Should have lowercase or uppercase ASCII letters, numbers, or dashes");

What about `.` and `_`? If I'm understanding `getAcceptableRange` correctly, these are also allowed?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1213#pullrequestreview-123563458

Re: [jclouds/jclouds] Adds new more relaxed validator for Azure entities (#1213)

[Andrew Phillips <no...@github.com>]

demobox commented on this pull request.



> +/**
+ * Validates name for azure entities
+ * https://docs.microsoft.com/en-us/azure/architecture/best-practices/naming-conventions
+ *
+ * @see org.jclouds.predicates.Validator
+ */
+@Singleton
+public class AzureNameValidator extends Validator<String> {
+   private final int min = 2;
+   private final int max = 63;
+
+   public void validate(String name) {
+
+      if (name == null || name.length() < min || name.length() > max)
+         throw exception(name, "Can't be null or empty. Length must be " + min + " to " + max + " symbols.");
+      if (CharMatcher.JAVA_LETTER_OR_DIGIT.indexIn(name) != 0)

[minor] `!CharMatcher.JAVA_LETTER_OR_DIGIT.matches(name.charAt(0))` expresses the intent more clearly?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1213#pullrequestreview-123563424

Re: [jclouds/jclouds] Adds new more relaxed validator for Azure entities (#1213)

[Daniel Estévez <no...@github.com>]

Added a test! (and fixed an error message by the way) with all the main cases i could think of @nacx 
Also, i added this class under _org.jclouds.azurecompute.arm.config.*_ not sure if it's the best place so feel free to suggest any other one

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1213#issuecomment-391841803

Re: [jclouds/jclouds] Adds new more relaxed validator for Azure entities (#1213)

[Ignasi Barrera <no...@github.com>]

Please add unit tests that verify the validator and test the corner cases.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1213#issuecomment-391515264

Re: [jclouds/jclouds] Adds new more relaxed validator for Azure entities (#1213)

[Daniel Estévez <no...@github.com>]

Thanks for the suggestions @demobox, PR was already merged but i added those improvements here https://github.com/jclouds/jclouds/pull/1215

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1213#issuecomment-392917310

Re: [jclouds/jclouds] Adds new more relaxed validator for Azure entities (#1213)

[Ignasi Barrera <no...@github.com>]

nacx approved this pull request.





-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1213#pullrequestreview-123270045

Re: [jclouds/jclouds] Adds new more relaxed validator for Azure entities (#1213)

[Ignasi Barrera <no...@github.com>]

Closed #1213.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1213#event-1646000469