Address comments

packages/@aws-cdk/aws-servicediscovery/lib/http-namespace.ts

@@ -44,6 +44,9 @@ export class HttpNamespace extends NamespaceBase {
     this.type = NamespaceType.Http;
   }
 
+  /**
+   * Creates a service within the namespace
+   */
   public createService(id: string, props?: BaseServiceProps): Service {
     return new Service(this, id, {
       namespace: this,

packages/@aws-cdk/aws-servicediscovery/lib/instance.ts

@@ -21,7 +21,7 @@ export interface BaseInstanceProps {
   /**
    * The id of the instance resource
    *
-   * @default CloudFormation-generated name
+   * @default Automatically generated name
    */
   instanceId?: string;
 

packages/@aws-cdk/aws-servicediscovery/lib/ip-instance.ts

@@ -58,12 +58,12 @@ export class IpInstance extends InstanceBase {
   public readonly service: IService;
 
   /**
-   * The Ipv4 address of the instance
+   * The Ipv4 address of the instance, or blank string if none available
    */
   public readonly ipv4: string;
 
   /**
-   * The Ipv6 address of the instance
+   * The Ipv6 address of the instance, or blank string if none available
    */
   public readonly ipv6: string;
 
@@ -76,6 +76,9 @@ export class IpInstance extends InstanceBase {
     super(scope, id);
     const dnsRecordType = props.service.dnsRecordType;
 
+    if (dnsRecordType === DnsRecordType.Cname) {
+        throw new Error('Service must support `A`, `AAAA` or `SRV` records to register this instance type.');
+    }
     if (dnsRecordType === DnsRecordType.Srv) {
       if (!props.port) {
         throw new Error('A `port` must be specified for a service using a `SRV` record.');
@@ -95,7 +98,7 @@ export class IpInstance extends InstanceBase {
       throw new Error('An `ipv6` must be specified for a service using a `AAAA` record.');
     }
 
-    const port = props.port !== undefined ? props.port : 80;
+    const port = props.port || 80;
 
     const resource = new CfnInstance(this, 'Resource', {
       instanceAttributes: {

packages/@aws-cdk/aws-servicediscovery/lib/namespace.ts

@@ -17,7 +17,7 @@ export interface INamespace extends cdk.IConstruct {
   readonly namespaceArn: string;
 
   /**
-   * Type of Namespace. Valid values: HTTP, DNS_PUBLIC, or DNS_PRIVATE
+   * Type of Namespace
    */
   readonly type: NamespaceType;
 }
@@ -84,8 +84,7 @@ export abstract class NamespaceBase extends cdk.Construct implements INamespace
   public abstract readonly type: NamespaceType;
 }
 
-// FIXME don't export
-export class ImportedNamespace extends NamespaceBase {
+class ImportedNamespace extends NamespaceBase {
   public readonly namespaceId: string;
   public readonly namespaceArn: string;
   public readonly namespaceName: string;

packages/@aws-cdk/aws-servicediscovery/lib/non-ip-instance.ts

@@ -40,7 +40,7 @@ export class NonIpInstance extends InstanceBase {
       throw new Error('This type of instance can only be registered for HTTP namespaces.');
     }
 
-    if (props.customAttributes === undefined) {
+    if (props.customAttributes === undefined || Object.keys(props.customAttributes).length === 0) {
       throw new Error('You must specify at least one custom attribute for this instance type.');
     }
 

packages/@aws-cdk/aws-servicediscovery/lib/private-dns-namespace.ts

@@ -59,6 +59,9 @@ export class PrivateDnsNamespace extends NamespaceBase {
     this.vpc = props.vpc;
   }
 
+  /**
+   * Creates a service within the namespace
+   */
   public createService(id: string, props?: DnsServiceProps): Service {
     return new Service(this, id, {
       namespace: this,

packages/@aws-cdk/aws-servicediscovery/lib/public-dns-namespace.ts

@@ -44,6 +44,9 @@ export class PublicDnsNamespace extends NamespaceBase {
     this.type = NamespaceType.DnsPublic;
   }
 
+  /**
+   * Creates a service within the namespace
+   */
   public createService(id: string, props?: DnsServiceProps): Service {
     return new Service(this, id, {
       namespace: this,

packages/@aws-cdk/aws-servicediscovery/lib/service.ts

@@ -38,8 +38,6 @@ export interface IService extends cdk.IConstruct {
    * The Routing Policy used by the service
    */
   readonly routingPolicy: RoutingPolicy;
-
-  // Possibly add boolean loadbalancer field?
 }
 
 /**
@@ -92,10 +90,9 @@ export interface DnsServiceProps extends BaseServiceProps {
   dnsTtlSec?: number;
 
   /**
-   * A complex type that contains settings for an optional health check.
-   * If you specify settings for a health check, AWS Cloud Map associates the health check with the
-   * records that you specify in DnsConfig.
-   * Public DNS namespaces only. Only one of healthCheckConfig or healthCheckCustomConfig can be specified.
+   * Settings for an optional health check.  If you specify health check settings, AWS Cloud Map associates the
+   * health check with the records that you specify in DnsConfig. Public DNS namespaces only. Only one of
+   * healthCheckConfig or healthCheckCustomConfig can be specified.
    *
    * @default none
    */
@@ -114,7 +111,6 @@ export interface DnsServiceProps extends BaseServiceProps {
    *
    * @default false
    */
-
   loadBalancer?: boolean;
 }
 
@@ -159,7 +155,6 @@ export class Service extends cdk.Construct implements IService {
    */
   public readonly routingPolicy: RoutingPolicy;
 
-  // FIXME make this only called through #createService on namespace classes?
   constructor(scope: cdk.Construct, id: string, props: ServiceProps) {
     super(scope, id);
 
@@ -199,7 +194,7 @@ export class Service extends cdk.Construct implements IService {
       ? RoutingPolicy.Weighted
       : RoutingPolicy.Multivalue;
 
-    const dnsRecordType = props.dnsRecordType !== undefined ? props.dnsRecordType : DnsRecordType.A;
+    const dnsRecordType = props.dnsRecordType || DnsRecordType.A;
 
     if (props.loadBalancer
       && (!(dnsRecordType === DnsRecordType.A
@@ -211,7 +206,7 @@ export class Service extends cdk.Construct implements IService {
     const dnsConfig = props.namespace.type === NamespaceType.Http
       ? undefined
       : {
-          dnsRecords: _getDnsRecords(dnsRecordType, props.dnsTtlSec),
+          dnsRecords: renderDnsRecords(dnsRecordType, props.dnsTtlSec),
           namespaceId: props.namespace.namespaceId,
           routingPolicy,
         };
@@ -224,13 +219,8 @@ export class Service extends cdk.Construct implements IService {
         : undefined
     };
 
-    const healthCheckConfig = props.healthCheckConfig
-        ? { ...healthCheckConfigDefaults, ...props.healthCheckConfig }
-        : undefined;
-
-    const healthCheckCustomConfig = props.healthCheckCustomConfig
-        ? props.healthCheckCustomConfig
-        : undefined;
+    const healthCheckConfig = props.healthCheckConfig && { ...healthCheckConfigDefaults, ...props.healthCheckConfig };
+    const healthCheckCustomConfig = props.healthCheckCustomConfig;
 
     // Create service
     const service = new CfnService(this, 'Resource', {
@@ -261,6 +251,9 @@ export class Service extends cdk.Construct implements IService {
     });
   }
 
+  /**
+   * Registers a resource that is accessible using values other than an IP address or a domain name (CNAME).
+   */
   public registerNonIpInstance(props: NonIpInstanceBaseProps): IInstance {
     return new NonIpInstance(this, "NonIpInstance", {
       service: this,
@@ -269,6 +262,9 @@ export class Service extends cdk.Construct implements IService {
     });
   }
 
+  /**
+   * Registers a resource that is accessible using an IP address.
+   */
   public registerIpInstance(props: IpInstanceBaseProps): IInstance {
     return new IpInstance(this, "IpInstance", {
       service: this,
@@ -280,6 +276,9 @@ export class Service extends cdk.Construct implements IService {
     });
   }
 
+  /**
+   * Registers a resource that is accessible using a CNAME.
+   */
   public registerCnameInstance(props: CnameInstanceBaseProps): IInstance {
     return new CnameInstance(this, "CnameInstance", {
       service: this,
@@ -290,7 +289,7 @@ export class Service extends cdk.Construct implements IService {
   }
 }
 
-function _getDnsRecords(dnsRecordType: DnsRecordType, dnsTtlSec?: number): DnsRecord[] {
+function renderDnsRecords(dnsRecordType: DnsRecordType, dnsTtlSec?: number): DnsRecord[] {
   const ttl = dnsTtlSec !== undefined ? dnsTtlSec.toString() : '60';
 
   if (dnsRecordType === DnsRecordType.A_AAAA) {

packages/@aws-cdk/aws-servicediscovery/test/test.instance.ts

@@ -213,6 +213,30 @@ export = {
     test.done();
   },
 
+  'Registering IpInstance throws with wrong DNS record type'(test: Test) {
+    // GIVEN
+    const stack = new cdk.Stack();
+
+    const namespace = new servicediscovery.PublicDnsNamespace(stack, 'MyNamespace', {
+      name: 'dns',
+    });
+
+    const service = namespace.createService('MyService', {
+      name: 'service',
+      dnsRecordType: servicediscovery.DnsRecordType.Cname
+    });
+
+    // THEN
+    test.throws(() => {
+      service.registerIpInstance({
+        port: 3306
+      });
+    }, /Service must support `A`, `AAAA` or `SRV` records to register this instance type./);
+
+    test.done();
+  },
+
+
   'Registering AliasTargetInstance'(test: Test) {
     // GIVEN
     const stack = new cdk.Stack();
@@ -430,4 +454,25 @@ export = {
 
     test.done();
   },
+
+  'Throws when custom attribues are emptyfor NonIpInstance'(test: Test) {
+    // GIVEN
+    const stack = new cdk.Stack();
+
+    const namespace = new servicediscovery.HttpNamespace(stack, 'MyNamespace', {
+      name: 'http',
+    });
+
+    const service = namespace.createService('MyService');
+
+    // THEN
+    test.throws(() => {
+      service.registerNonIpInstance({
+        instanceId: 'nonIp',
+        customAttributes: {}
+      });
+    }, /You must specify at least one custom attribute for this instance type./);
+
+    test.done();
+  },
 };