mitchell852 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467169497
##########
File path: traffic_ops/traffic_ops_golang/staticdnsentry/staticdnsentry.go
##########
@@ -103,6 +104,11 @@ func (staticDNSEntry TOStaticDNSEntry) Validate() error {
addressErr = validation.Validate(staticDNSEntry.Address,
validation.Required, is.IPv6)
case "CNAME_RECORD":
addressErr = validation.Validate(staticDNSEntry.Address,
validation.Required, is.DNSName)
+ address := *staticDNSEntry.Address
+ lastChar := address[len(address)-1:]
+ if lastChar != "." && addressErr == nil {
+ addressErr = fmt.Errorf("must be a valid DNS name.
Missing a trailing period at the end")
Review comment:
how about something like
`CNAME address must have a trailing period.` (or is it called a dot?? not
sure.)
that way i don't feel like i made 2 mistakes.
##########
File path:
traffic_portal/app/src/common/modules/form/deliveryServiceStaticDnsEntry/form.deliveryServiceStaticDnsEntry.tpl.html
##########
@@ -57,9 +57,12 @@
</div>
</div>
<div class="form-group" ng-class="{'has-error':
hasError(dsStaticDnsEntryForm.address), 'has-feedback':
hasError(dsStaticDnsEntryForm.address)}">
- <label for="address" class="control-label col-md-2 col-sm-2
col-xs-12">Address *</label>
+ <label for="address" class="has-tooltip control-label col-md-2
col-sm-2 col-xs-12">Address *<div class="helptooltip">
+ <div class="helptext">The Address Rules.<br>1.
Type:A_RECORD, Address should be an IPv4 address. <br>2. Type: AAAA_RECORD,
Address should be an IPv6 address. <br>3. Type: CNAME, Address must end with a
trailing period if type:CNAME. Eg: cdn.test.com. is correct but cdn.test.com
will not work</div>
Review comment:
this description seems a little redundant:
`Type: CNAME, Address must end with a trailing period if type:CNAME` also it
doesn't describe the valid dns part. how about:
`Type: CNAME, Address must be a valid DNS name with a trailing period`
not even sure you need the example tbh.
also, that line is pretty long. want to break it up?
##########
File path: traffic_portal/test/end_to_end/deliveryServices/pageData.js
##########
@@ -37,6 +39,10 @@ module.exports = function(){
this.protocol=element(by.name('protocol'));
this.longDesc=element(by.name('longDesc'));
this.remapText=element(by.name('remapText'));
+ this.host=element(by.name('host'));
Review comment:
can you put these in their own section at the bottom or something? with
a comment over them along the lines of:
`// delivery service static dns entry fields`
otherwise, they look like ds properties which they are not.
##########
File path: docs/source/api/v2/staticdnsentries.rst
##########
@@ -314,7 +314,7 @@ Response Structure
"lastUpdated": "2018-12-10 19:59:56+00",
"ttl": 300,
"type": null,
Review comment:
can. you fix type to not be null? i think type is supposed to be `type:
CNAME_RECORD`
##########
File path: docs/source/api/v2/staticdnsentries.rst
##########
@@ -133,7 +133,7 @@ Creates a new, static DNS entry.
Request Structure
-----------------
-:address: If ``typeId`` identifies a ``CNAME`` type record, this is the
Canonical Name (CNAME) of the server, otherwise it is the IP address to which
``host`` shall be resolved
+:address: If ``typeId`` identifies a ``CNAME`` type record, this is the
Canonical Name (CNAME) of the server that is required to end with a trailing
period (eg:cdn.test.com. is the right usage but cdn.test.com will throw an
error), otherwise it is the IP address to which ``host`` shall be resolved
Review comment:
this description seems different than the rest. i think i like the
simpler descriptions without the example as i think it's obvious.
##########
File path: traffic_portal/test/end_to_end/deliveryServices/pageData.js
##########
@@ -37,6 +39,10 @@ module.exports = function(){
this.protocol=element(by.name('protocol'));
this.longDesc=element(by.name('longDesc'));
this.remapText=element(by.name('remapText'));
+ this.host=element(by.name('host'));
+ this.typeId=element(by.name('typeId'));
Review comment:
and maybe rename this one to `staticDNSTypeId` otherwise it looks like
it's the delivery service type.
##########
File path:
traffic_portal/test/end_to_end/deliveryServices/delivery-services-spec.js
##########
@@ -327,6 +331,30 @@ describe('Traffic Portal Delivery Services Suite',
function() {
});
});
+ it('should add a required Static DNS entry to the HTTP delivery
service', function() {
+ pageData.dsLink.click();
+ console.log('Adding Static DNS entry to ' + mockVals.httpXmlId);
+ pageData.moreBtn.click();
+ pageData.viewStaticCapabilitiesMenuItem.click();
+
expect(browser.getCurrentUrl().then(commonFunctions.urlPath)).toMatch(commonFunctions.urlPath(browser.baseUrl)+"#!/delivery-services/[0-9]+/static-dns-entries");
+ pageData.addStaticDNSBtn.click();
+ //
expect(pageData.selectFormSubmitButton.isEnabled()).toBe(false);
+ // set host name
+ pageData.host.sendKeys(mockVals.staticDNShostName);
+ // set type ID
Review comment:
want to say in your comment: `set type to CNAME_RECORD's id` or
something so it's clear what `3` is.
##########
File path: docs/source/api/v3/staticdnsentries.rst
##########
@@ -234,7 +234,7 @@ Request Structure
| id | The integral, unique identifier of the static DNS entry to
modify |
+------+-------------------------------------------------------------------+
-:address: If ``typeId`` identifies a ``CNAME`` type record, this is
the Canonical Name (CNAME) of the server, otherwise it is the IP address to
which ``host`` shall be resolved
+:address: If ``typeId`` identifies a ``CNAME`` type record, this is
the Canonical Name (CNAME) of the server that is required to end with a
trailing period (eg:cdn.test.com. is right usage but cdn.test.com will throw an
error), otherwise it is the IP address to which ``host`` shall be resolved
Review comment:
again. make the descriptions consistent.
##########
File path:
traffic_portal/test/end_to_end/deliveryServices/delivery-services-spec.js
##########
@@ -327,6 +331,30 @@ describe('Traffic Portal Delivery Services Suite',
function() {
});
});
+ it('should add a required Static DNS entry to the HTTP delivery
service', function() {
+ pageData.dsLink.click();
+ console.log('Adding Static DNS entry to ' + mockVals.httpXmlId);
+ pageData.moreBtn.click();
+ pageData.viewStaticCapabilitiesMenuItem.click();
+
expect(browser.getCurrentUrl().then(commonFunctions.urlPath)).toMatch(commonFunctions.urlPath(browser.baseUrl)+"#!/delivery-services/[0-9]+/static-dns-entries");
+ pageData.addStaticDNSBtn.click();
+ //
expect(pageData.selectFormSubmitButton.isEnabled()).toBe(false);
Review comment:
want to delete this commented out line?
##########
File path: traffic_ops/traffic_ops_golang/staticdnsentry/staticdnsentry.go
##########
@@ -103,6 +104,11 @@ func (staticDNSEntry TOStaticDNSEntry) Validate() error {
addressErr = validation.Validate(staticDNSEntry.Address,
validation.Required, is.IPv6)
case "CNAME_RECORD":
addressErr = validation.Validate(staticDNSEntry.Address,
validation.Required, is.DNSName)
+ address := *staticDNSEntry.Address
+ lastChar := address[len(address)-1:]
Review comment:
is it possible for `address` to have a length of 0? if so, this will
fail, right?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
