App Service: Domain CRUD #1254
App Service: Domain CRUD #1254
Conversation
| */ | ||
| public final class CountryISOCode { | ||
| // CHECKSTYLE IGNORE Javadoc FOR NEXT 237 LINES | ||
| public static final CountryISOCode Afghanistan = new CountryISOCode("AF"); |
There was a problem hiding this comment.
conventions - all caps for enum constants (or static public fields) - AFGHANISTAN, ALBANIA, etc.
| // CHECKSTYLE IGNORE Javadoc FOR NEXT 237 LINES | ||
| public static final CountryISOCode Afghanistan = new CountryISOCode("AF"); | ||
| public static final CountryISOCode Albania = new CountryISOCode("AL"); | ||
| public static final CountryISOCode Algeria = new CountryISOCode("DZ"); |
There was a problem hiding this comment.
conventions - this should be all caps (AFGHANISTAN, ALBANIA...)
| // CHECKSTYLE IGNORE Javadoc FOR NEXT 236 LINES | ||
| public static final CountryPhoneCode Albania = new CountryPhoneCode("+355"); | ||
| public static final CountryPhoneCode Algeria = new CountryPhoneCode("+213"); | ||
| public static final CountryPhoneCode Andorra = new CountryPhoneCode("+376"); |
There was a problem hiding this comment.
all caps - i know this is auto-gen'd - don;'t we have control over capitalization of extensible enums?
| /** | ||
| * @return all hostnames derived from the domain and assigned to Azure resources | ||
| */ | ||
| List<HostName> managedHostNames(); |
There was a problem hiding this comment.
how about Map<String, HostName> --- isn't there some useful unique string to index by?
There was a problem hiding this comment.
Azure DNS allows multiple mappings from a host name, for example, "www" can point to multiple IP addresses. (Probably for load balancing)
| * @return hostnames for the web app that are enabled. Hostnames need to be | ||
| * assigned and enabled. If some hostnames are assigned but not enabled | ||
| * the app is not served on those hostnames. | ||
| * @return host names for the web app that are enabled | ||
| */ | ||
| List<String> enabledHostNames(); |
There was a problem hiding this comment.
i assume the strings in this are unique - if so how about Set?
There was a problem hiding this comment.
Will need to be verified together with the previous.
| @@ -194,7 +168,7 @@ | |||
| */ | |||
| String defaultHostName(); | |||
|
|
|||
| List<HostNameBinding> hostNameBindings() throws CloudException, IOException; | |||
| List<HostNameBinding> getHostNameBindings(); | |||
There was a problem hiding this comment.
is there a unique string that could be used as the index if this returned Map instead of List?
There was a problem hiding this comment.
This one, however, seems possible. There is probably only one binding from one host to one web app.
| @@ -194,7 +168,7 @@ | |||
| */ | |||
| String defaultHostName(); | |||
|
|
|||
| List<HostNameBinding> hostNameBindings() throws CloudException, IOException; | |||
| List<HostNameBinding> getHostNameBindings(); | |||
There was a problem hiding this comment.
is there a unique string that could be used as the index if this returned Map<String, HostNameBinding> instead of List?
| .withNewAppServicePlan("java-plan-323", AppServicePricingTier.STANDARD_S1) | ||
| .defineHostNameBinding(domain, "test2.javatestpr319.com") | ||
| .withHostNameType(HostNameType.MANAGED) | ||
| .withHostNameDnsRecordType(CustomHostNameDnsRecordType.A) |
There was a problem hiding this comment.
just a thought - perhaps this could be just withDnsRecordType? Is it ambiguous?
There was a problem hiding this comment.
Web app fluent design is being overhauled.
|
Can we merge this yet? The country codes are useful for CDN too. |
App Service: Domain CRUD
* Fixing minor validation errors - Fixing description rule to allow references to define a description - Fixing AutoRest logging output to avoid throwing NRE when an error is logged without an exception - Changing the message that gets output for validation errors to make it more useful * Adding a validation test that validates a clean spec - This verifies that rules are not unintentionally being returned for specs that don't have the issue. If a rule is added that causes this test to fail, this spec should be updated so that it passes those rules (and verify that the rule isn't being triggered inadvertently)
Known issue: javadocs missing
Justification: generated code is failing checkstyle. Will fix after AutoRest refactoring is done.