Re: [jclouds/jclouds] Adds new more relaxed validator for Azure entities (#1213)
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)
demobox commented on this pull request.
> + */
+@Singleton
+public class AzureNameValidator extends Validator {
+ 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)
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 {
+ 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)
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 {
+ 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)
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)
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
Re: [jclouds/jclouds] Adds new more relaxed validator for Azure entities (#1213)
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)
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)
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
[jclouds/jclouds] Adds new more relaxed validator for Azure entities (#1213)
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
