You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2020/08/03 15:34:02 UTC

[GitHub] [trafficcontrol] rimashah25 opened a new pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

rimashah25 opened a new pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934


   <!--
   ************ STOP!! ************
   If this Pull Request is intended to fix a security vulnerability, DO NOT submit it! Instead, contact
   the Apache Software Foundation Security Team at security@trafficcontrol.apache.org and follow the
   guidelines at https://www.apache.org/security/ regarding vulnerability disclosure.
   -->
   ## What does this PR (Pull Request) do?
   <!-- Explain the changes you made here. If this fixes an Issue, identify it by
   replacing the text in the checkbox item with the Issue number e.g.
   
   - [x] This PR fixes #9001 OR is not related to any Issue
   
   ^ This will automatically close Issue number 9001 when the Pull Request is
   merged (The '#' is important).
   
   Be sure you check the box properly, see the "The following criteria are ALL
   met by this PR" section for details.
   -->
   
   - [x] This PR fixes a bug in Traffic_Portal's UI. Github Issue: #4743 (https://github.com/apache/trafficcontrol/issues/4743)
   
   ## Which Traffic Control components are affected by this PR?
   <!-- Please delete all components from this list that are NOT affected by this
   Pull Request. Also, feel free to add the name of a tool or script that is
   affected but not on the list.
   
   Additionally, if this Pull Request does NOT affect documentation, please
   explain why documentation is not required. -->
   
   - Traffic Portal
   
   ## What is the best way to verify this PR?
   <!-- Please include here ALL the steps necessary to test your Pull Request. If
   it includes tests (and most should), outline here the steps needed to run the
   tests. If not, lay out the manual testing procedure and please explain why
   tests are unnecessary for this Pull Request. -->
   Manual Testing
   1. Create a HTTP delivery service
   2. Click on More and then add Static DNS entry
   3. Select a Type and type address without a trailing period. Notice the create button isn't green
   4. Once you do add a trailing period, create button turn green and you can create a static DNS entry.
   Automation Testing
   1. Run CIAB
   2. Run Traffic_Portal locally (make sure the Traffic_Ops url point to CIAB traffic ops url)
   3. Ensure protractor and webdriver-manage are installed.
   4. Run webdriver-manager start
   5. Run protractor config.js from traffic_control/traffic_portal/test/end-to-end/ folder
   6. All 74 specs should pass. `74 specs, 0 failures`
   
   ## If this is a bug fix, what versions of Traffic Control are affected?
   <!-- If this PR fixes a bug, please list here all of the affected versions - to
   the best of your knowledge. It's also pretty helpful to include a commit hash
   of where 'master' is at the time this PR is opened (if it affects master),
   because what 'master' means will change over time. For example, if this PR
   fixes a bug that's present in master (at commit hash '1df853c8'), in v4.0.0,
   and in the current 4.0.1 Release candidate (e.g. RC1), then this list would
   look like:
   
   - master
   
   If you don't know what other versions might have this bug, AND don't know how
   to find the commit hash of 'master', then feel free to leave this section
   blank (or, preferably, delete it entirely).
    -->
   
   - master (e5d7dc8)
   
   ## The following criteria are ALL met by this PR
   <!-- Check the boxes to signify that the associated statement is true. To
   "check a box", replace the space inside of the square brackets with an 'x'.
   e.g.
   
   - [ x] <- Wrong
   - [x ] <- Wrong
   - [] <- Wrong
   - [*] <- Wrong
   - [x] <- Correct!
   
   -->
   
   - [x] This PR includes tests
   - [ ] This PR does not include documentation
   - [x] This PR includes an update to CHANGELOG.md OR such an update is not necessary
   - [x] This PR includes any and all required license headers
   - [x] This does not include a database migration
   - [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
   
   
   ## Additional Information
   <!-- If you would like to include any additional information on the PR for
   potential reviewers please put it here.
   
   Some examples of this would be:
   
   - Before and after screenshots/gifs of the Traffic Portal if it is affected
   - Links to other dependent Pull Requests
   - References to relevant context (e.g. new/updates to dependent libraries,
   mailing list records, blueprints)
   
   Feel free to leave this section blank (or, preferably, delete it entirely).
   -->
   
   <!--
   Licensed to the Apache Software Foundation (ASF) under one
   or more contributor license agreements.  See the NOTICE file
   distributed with this work for additional information
   regarding copyright ownership.  The ASF licenses this file
   to you under the Apache License, Version 2.0 (the
   "License"); you may not use this file except in compliance
   with the License.  You may obtain a copy of the License at
   
       http://www.apache.org/licenses/LICENSE-2.0
   
   Unless required by applicable law or agreed to in writing,
   software distributed under the License is distributed on an
   "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
   KIND, either express or implied.  See the License for the
   specific language governing permissions and limitations
   under the License.
   -->


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467223775



##########
File path: traffic_ops/traffic_ops_golang/staticdnsentry/staticdnsentry.go
##########
@@ -103,6 +104,13 @@ 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
+		if len(address) != 0 {

Review comment:
       why check anything if len(address) == 0? If len != 0, only then I want to check addressErr and trailing period.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467198001



##########
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:
       > also, there are 4 types, right?
   > 
   > ![image](https://user-images.githubusercontent.com/251272/89673576-d4391100-d8a3-11ea-90a1-e946a0ca5b00.png)
   > 
   > are there any rules for TXT_RECORD? if so, want to add it to your tooltip?
   > 
   > ![image](https://user-images.githubusercontent.com/251272/89673532-c7b4b880-d8a3-11ea-9629-fca88d1ba1fe.png)
   
   Code updated to reflect TXT_RECORD
   ![image](https://user-images.githubusercontent.com/22248619/89676308-b0c49500-d8a8-11ea-9c1d-a463c6c2482d.png)
   




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467179716



##########
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:
       Yeah, i'll change it.
   That part was a previous code when I had a different sentence. Sorry about that.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467219986



##########
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:
       Updated the code to ensure the trailing period check takes place only if address length is greater than zero
   ![image](https://user-images.githubusercontent.com/22248619/89679579-a1484a80-d8ae-11ea-807a-84ba99e1c394.png)
   




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467197613



##########
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:
       Code updated.

##########
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:
       Code updated.

##########
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:
       Code updated.

##########
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:
       Code updated.

##########
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:
       > also, there are 4 types, right?
   > 
   > ![image](https://user-images.githubusercontent.com/251272/89673576-d4391100-d8a3-11ea-90a1-e946a0ca5b00.png)
   > 
   > are there any rules for TXT_RECORD? if so, want to add it to your tooltip?
   > 
   > ![image](https://user-images.githubusercontent.com/251272/89673532-c7b4b880-d8a3-11ea-9629-fca88d1ba1fe.png)
   
   Code updated.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467213114



##########
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:
       actually, even though it is required, it looks to me like it will still run this line of code:
   
   `
   lastChar := address[len(address)-1:]
   `




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r468744794



##########
File path: traffic_portal/app/src/common/modules/form/deliveryServiceStaticDnsEntry/form.deliveryServiceStaticDnsEntry.tpl.html
##########
@@ -57,9 +57,20 @@
                 </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.
+                        <ul>
+                            <li>Type:A_RECORD, address should be an IPv4 address.</li>
+                            <li>Type:AAAA_RECORD, address should be an IPv6 address.</li>
+                            <li>Type:CNAME, address must end with a trailing period.</li>
+                            <li>Type:TXT_RECORD, address cannot be blank.</li>
+                        </ul>
+                    </div>
+                </div>
+                </label>
                 <div class="col-md-10 col-sm-10 col-xs-12">
-                    <input id="address" name="address" type="text" class="form-control" ng-model="staticDnsEntry.address" required>
+                    <input id="address" name="address" type="text" class="form-control" ng-model="staticDnsEntry.address"
+                           required title= "Address must be: an IPv4, if type:A_RECORD; an IPv6, if type: AAAA_RECORD; end with a trailing period if type:CNAME_RECORD; cannot be blank; if type: TXT_RECORD">

Review comment:
       Completed.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467223775



##########
File path: traffic_ops/traffic_ops_golang/staticdnsentry/staticdnsentry.go
##########
@@ -103,6 +104,13 @@ 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
+		if len(address) != 0 {

Review comment:
       why check anything if len(address) = 0? If len != 0, only then I want to check addressErr and for trailing period.

##########
File path: traffic_ops/traffic_ops_golang/staticdnsentry/staticdnsentry.go
##########
@@ -103,6 +104,13 @@ 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
+		if len(address) != 0 {

Review comment:
       why check anything if len(address) = 0? If len != 0, only then I want to check addressErr and trailing period.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467177804



##########
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 `typeId` to `staticDNSTypeId` otherwise it looks like it's the delivery service type.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467198265



##########
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:
       Code updated.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r468680215



##########
File path: traffic_portal/app/src/common/modules/form/deliveryServiceStaticDnsEntry/form.deliveryServiceStaticDnsEntry.tpl.html
##########
@@ -57,9 +57,20 @@
                 </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.
+                        <ul>
+                            <li>Type:A_RECORD, address should be an IPv4 address.</li>
+                            <li>Type:AAAA_RECORD, address should be an IPv6 address.</li>
+                            <li>Type:CNAME, address must end with a trailing period.</li>
+                            <li>Type:TXT_RECORD, address cannot be blank.</li>
+                        </ul>
+                    </div>
+                </div>
+                </label>
                 <div class="col-md-10 col-sm-10 col-xs-12">
-                    <input id="address" name="address" type="text" class="form-control" ng-model="staticDnsEntry.address" required>
+                    <input id="address" name="address" type="text" class="form-control" ng-model="staticDnsEntry.address"
+                           required title= "Address must be: an IPv4, if type:A_RECORD; an IPv6, if type: AAAA_RECORD; end with a trailing period if type:CNAME_RECORD; cannot be blank; if type: TXT_RECORD">

Review comment:
       Umm.. this is an attribute field.. I am not sure you can add "<!--ul-->" and "<!--li-->" tags within an input attribute field 




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467222087



##########
File path: traffic_portal/app/src/common/modules/form/deliveryServiceStaticDnsEntry/form.deliveryServiceStaticDnsEntry.tpl.html
##########
@@ -57,9 +57,18 @@
                 </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.
+                        <br>4. Type:TXT_RECORD, address cannot be blank.
+                    </div>
+                </div>
+                </label>
                 <div class="col-md-10 col-sm-10 col-xs-12">
-                    <input id="address" name="address" type="text" class="form-control" ng-model="staticDnsEntry.address" required>
+                    <input id="address" name="address" type="text" class="form-control" ng-model="staticDnsEntry.address"
+                           required title= "Address must be: an IPv4, if type:A_RECORD; an IPv6, if type: AAAA_RECORD; end with a trailing period if type:CNAME_RECORD; cannot be blank; if type: TXT_RECORD">

Review comment:
       Yup a lot of them do.. It helps the user no matter where they are hovering, name, textbox
   ![image](https://user-images.githubusercontent.com/22248619/89679976-5844c600-d8af-11ea-8de9-c5767d7a9f21.png)
   




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467170690



##########
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 div line is pretty long. want to break it up?




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467197585



##########
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:
       Change completed.

##########
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:
       Change completed.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r468680215



##########
File path: traffic_portal/app/src/common/modules/form/deliveryServiceStaticDnsEntry/form.deliveryServiceStaticDnsEntry.tpl.html
##########
@@ -57,9 +57,20 @@
                 </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.
+                        <ul>
+                            <li>Type:A_RECORD, address should be an IPv4 address.</li>
+                            <li>Type:AAAA_RECORD, address should be an IPv6 address.</li>
+                            <li>Type:CNAME, address must end with a trailing period.</li>
+                            <li>Type:TXT_RECORD, address cannot be blank.</li>
+                        </ul>
+                    </div>
+                </div>
+                </label>
                 <div class="col-md-10 col-sm-10 col-xs-12">
-                    <input id="address" name="address" type="text" class="form-control" ng-model="staticDnsEntry.address" required>
+                    <input id="address" name="address" type="text" class="form-control" ng-model="staticDnsEntry.address"
+                           required title= "Address must be: an IPv4, if type:A_RECORD; an IPv6, if type: AAAA_RECORD; end with a trailing period if type:CNAME_RECORD; cannot be blank; if type: TXT_RECORD">

Review comment:
       Umm.. this is an attribute field.. I am not sure you can add "ul" and "li" tags within an input attribute field 




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467521334



##########
File path: traffic_portal/app/src/common/modules/form/deliveryServiceStaticDnsEntry/form.deliveryServiceStaticDnsEntry.tpl.html
##########
@@ -58,11 +58,17 @@
             </div>
             <div class="form-group" ng-class="{'has-error': hasError(dsStaticDnsEntryForm.address), 'has-feedback': hasError(dsStaticDnsEntryForm.address)}">
                 <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>
+                    <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.
+                        <br>4. Type:TXT_RECORD, address cannot be blank.

Review comment:
       instead of manually numbering and line-breaking, this should probably be a list. But also, I don't think the order of the rules is important, so it should probably be an unordered list:
   
   ```html
   <div class="helptext">The Address rules:
     <ul>
       <li>Type:A_RECORD - address should be an IPv4 address</li>
       <li>Type:AAAA_RECORD - address should be an IPv6 address.</li>
       <li>Type:CNAME - address must end with a trailing period.</li>
       <li>Type:TXT_RECORD - address cannot be blank.</li>
     </ul>
   </div>
   ```




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r468655605



##########
File path: traffic_portal/app/src/common/modules/form/deliveryServiceStaticDnsEntry/form.deliveryServiceStaticDnsEntry.tpl.html
##########
@@ -57,9 +57,20 @@
                 </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.
+                        <ul>
+                            <li>Type:A_RECORD, address should be an IPv4 address.</li>
+                            <li>Type:AAAA_RECORD, address should be an IPv6 address.</li>
+                            <li>Type:CNAME, address must end with a trailing period.</li>

Review comment:
       can you change this to:
   
   `Type:CNAME, address must be _a valid DNS name_ ending with a trailing period.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r464690895



##########
File path: traffic_portal/app/src/common/service/utils/LocationUtils.js
##########
@@ -19,6 +19,11 @@
 
 var LocationUtils = function($location, $uibModal) {
 
+    // ... this one allows Static DNS entries with only a trailing period
+    this.StaticDNSPattern = /^(?=.{1,253}\.?$)(?:(?!-|[^.]+_)[A-Za-z0-9-_]{1,63}(?:\.)){2,}$/;

Review comment:
       Ok




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r468680215



##########
File path: traffic_portal/app/src/common/modules/form/deliveryServiceStaticDnsEntry/form.deliveryServiceStaticDnsEntry.tpl.html
##########
@@ -57,9 +57,20 @@
                 </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.
+                        <ul>
+                            <li>Type:A_RECORD, address should be an IPv4 address.</li>
+                            <li>Type:AAAA_RECORD, address should be an IPv6 address.</li>
+                            <li>Type:CNAME, address must end with a trailing period.</li>
+                            <li>Type:TXT_RECORD, address cannot be blank.</li>
+                        </ul>
+                    </div>
+                </div>
+                </label>
                 <div class="col-md-10 col-sm-10 col-xs-12">
-                    <input id="address" name="address" type="text" class="form-control" ng-model="staticDnsEntry.address" required>
+                    <input id="address" name="address" type="text" class="form-control" ng-model="staticDnsEntry.address"
+                           required title= "Address must be: an IPv4, if type:A_RECORD; an IPv6, if type: AAAA_RECORD; end with a trailing period if type:CNAME_RECORD; cannot be blank; if type: TXT_RECORD">

Review comment:
       Umm.. this is an attribute field.. I am not sure you can add <ul> and <li> tags within an input attribute field 

##########
File path: traffic_portal/app/src/common/modules/form/deliveryServiceStaticDnsEntry/form.deliveryServiceStaticDnsEntry.tpl.html
##########
@@ -57,9 +57,20 @@
                 </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.
+                        <ul>
+                            <li>Type:A_RECORD, address should be an IPv4 address.</li>
+                            <li>Type:AAAA_RECORD, address should be an IPv6 address.</li>
+                            <li>Type:CNAME, address must end with a trailing period.</li>

Review comment:
       sigh.... done

##########
File path: traffic_portal/app/src/common/modules/form/deliveryServiceStaticDnsEntry/form.deliveryServiceStaticDnsEntry.tpl.html
##########
@@ -57,9 +57,20 @@
                 </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.
+                        <ul>
+                            <li>Type:A_RECORD, address should be an IPv4 address.</li>
+                            <li>Type:AAAA_RECORD, address should be an IPv6 address.</li>
+                            <li>Type:CNAME, address must end with a trailing period.</li>
+                            <li>Type:TXT_RECORD, address cannot be blank.</li>
+                        </ul>
+                    </div>
+                </div>
+                </label>
                 <div class="col-md-10 col-sm-10 col-xs-12">
-                    <input id="address" name="address" type="text" class="form-control" ng-model="staticDnsEntry.address" required>
+                    <input id="address" name="address" type="text" class="form-control" ng-model="staticDnsEntry.address"
+                           required title= "Address must be: an IPv4, if type:A_RECORD; an IPv6, if type: AAAA_RECORD; end with a trailing period if type:CNAME_RECORD; cannot be blank; if type: TXT_RECORD">

Review comment:
       Umm.. this is an attribute field.. I am not sure you can add "<ul>" and "<li>" tags within an input attribute field 




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467086777



##########
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>
+                </div>
+                </label>
                 <div class="col-md-10 col-sm-10 col-xs-12">
-                    <input id="address" name="address" type="text" class="form-control" ng-model="staticDnsEntry.address" required>
+                    <input id="address" name="address" type="text" class="form-control" ng-model="staticDnsEntry.address" ng-pattern="StaticDNSPattern" required title= "Address must be: an IPv4, if type:A_RECORD; be an IPv6, if type: AAAA_RECORD; end with a trailing period if type:CNAME_RECORD; ">

Review comment:
       I thought I removed it.. 
   Updated code.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467198376



##########
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:
       Code updated.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r468655605



##########
File path: traffic_portal/app/src/common/modules/form/deliveryServiceStaticDnsEntry/form.deliveryServiceStaticDnsEntry.tpl.html
##########
@@ -57,9 +57,20 @@
                 </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.
+                        <ul>
+                            <li>Type:A_RECORD, address should be an IPv4 address.</li>
+                            <li>Type:AAAA_RECORD, address should be an IPv6 address.</li>
+                            <li>Type:CNAME, address must end with a trailing period.</li>

Review comment:
       can you change this to:
   
   `Type:CNAME, address must be a valid DNS name ending with a trailing period.

##########
File path: traffic_portal/app/src/common/modules/form/deliveryServiceStaticDnsEntry/form.deliveryServiceStaticDnsEntry.tpl.html
##########
@@ -57,9 +57,20 @@
                 </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.
+                        <ul>
+                            <li>Type:A_RECORD, address should be an IPv4 address.</li>
+                            <li>Type:AAAA_RECORD, address should be an IPv6 address.</li>
+                            <li>Type:CNAME, address must end with a trailing period.</li>
+                            <li>Type:TXT_RECORD, address cannot be blank.</li>
+                        </ul>
+                    </div>
+                </div>
+                </label>
                 <div class="col-md-10 col-sm-10 col-xs-12">
-                    <input id="address" name="address" type="text" class="form-control" ng-model="staticDnsEntry.address" required>
+                    <input id="address" name="address" type="text" class="form-control" ng-model="staticDnsEntry.address"
+                           required title= "Address must be: an IPv4, if type:A_RECORD; an IPv6, if type: AAAA_RECORD; end with a trailing period if type:CNAME_RECORD; cannot be blank; if type: TXT_RECORD">

Review comment:
       and update it. here too?




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org



[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467214999



##########
File path: traffic_portal/app/src/common/modules/form/deliveryServiceStaticDnsEntry/form.deliveryServiceStaticDnsEntry.tpl.html
##########
@@ -57,9 +57,18 @@
                 </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.
+                        <br>4. Type:TXT_RECORD, address cannot be blank.
+                    </div>
+                </div>
+                </label>
                 <div class="col-md-10 col-sm-10 col-xs-12">
-                    <input id="address" name="address" type="text" class="form-control" ng-model="staticDnsEntry.address" required>
+                    <input id="address" name="address" type="text" class="form-control" ng-model="staticDnsEntry.address"
+                           required title= "Address must be: an IPv4, if type:A_RECORD; an IPv6, if type: AAAA_RECORD; end with a trailing period if type:CNAME_RECORD; cannot be blank; if type: TXT_RECORD">

Review comment:
       i don't think you need this `title` attribute on this input element. do other input elements have it?




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467074413



##########
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>
+                </div>
+                </label>
                 <div class="col-md-10 col-sm-10 col-xs-12">
-                    <input id="address" name="address" type="text" class="form-control" ng-model="staticDnsEntry.address" required>
+                    <input id="address" name="address" type="text" class="form-control" ng-model="staticDnsEntry.address" ng-pattern="StaticDNSPattern" required title= "Address must be: an IPv4, if type:A_RECORD; be an IPv6, if type: AAAA_RECORD; end with a trailing period if type:CNAME_RECORD; ">

Review comment:
       `ng-pattern="StaticDNSPattern"`
   
   doesn't that need to come out? ^^




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r468739823



##########
File path: traffic_portal/app/src/common/modules/form/deliveryServiceStaticDnsEntry/form.deliveryServiceStaticDnsEntry.tpl.html
##########
@@ -57,9 +57,20 @@
                 </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.
+                        <ul>
+                            <li>Type:A_RECORD, address should be an IPv4 address.</li>
+                            <li>Type:AAAA_RECORD, address should be an IPv6 address.</li>
+                            <li>Type:CNAME, address must end with a trailing period.</li>
+                            <li>Type:TXT_RECORD, address cannot be blank.</li>
+                        </ul>
+                    </div>
+                </div>
+                </label>
                 <div class="col-md-10 col-sm-10 col-xs-12">
-                    <input id="address" name="address" type="text" class="form-control" ng-model="staticDnsEntry.address" required>
+                    <input id="address" name="address" type="text" class="form-control" ng-model="staticDnsEntry.address"
+                           required title= "Address must be: an IPv4, if type:A_RECORD; an IPv6, if type: AAAA_RECORD; end with a trailing period if type:CNAME_RECORD; cannot be blank; if type: TXT_RECORD">

Review comment:
       no, i mean fix the description to 
   
   `valid DNS name with a trailing period if type:CNAME_RECORD`




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467227576



##########
File path: traffic_ops/traffic_ops_golang/staticdnsentry/staticdnsentry.go
##########
@@ -103,6 +104,13 @@ 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
+		if len(address) != 0 {

Review comment:
       updated




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] mitchell852 merged pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
mitchell852 merged pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934


   


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r468637439



##########
File path: traffic_portal/app/src/common/modules/form/deliveryServiceStaticDnsEntry/form.deliveryServiceStaticDnsEntry.tpl.html
##########
@@ -58,11 +58,17 @@
             </div>
             <div class="form-group" ng-class="{'has-error': hasError(dsStaticDnsEntryForm.address), 'has-feedback': hasError(dsStaticDnsEntryForm.address)}">
                 <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>
+                    <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.
+                        <br>4. Type:TXT_RECORD, address cannot be blank.

Review comment:
       Completed




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467179716



##########
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:
       Yeah, i'll change it.
   

##########
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:
       Ok!




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r464657207



##########
File path: traffic_portal/app/src/common/service/utils/LocationUtils.js
##########
@@ -19,6 +19,11 @@
 
 var LocationUtils = function($location, $uibModal) {
 
+    // ... this one allows Static DNS entries with only a trailing period
+    this.StaticDNSPattern = /^(?=.{1,253}\.?$)(?:(?!-|[^.]+_)[A-Za-z0-9-_]{1,63}(?:\.)){2,}$/;

Review comment:
       this probably makes more sense in DeliveryServiceUtils




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467198001



##########
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:
       > also, there are 4 types, right?
   > 
   > ![image](https://user-images.githubusercontent.com/251272/89673576-d4391100-d8a3-11ea-90a1-e946a0ca5b00.png)
   > 
   > are there any rules for TXT_RECORD? if so, want to add it to your tooltip?
   > 
   > ![image](https://user-images.githubusercontent.com/251272/89673532-c7b4b880-d8a3-11ea-9629-fca88d1ba1fe.png)
   
   Code updated to reflect TXT_RECORD




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] mitchell852 commented on pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
mitchell852 commented on pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#issuecomment-670634070


   also, there are 4 types, right?
   
   ![image](https://user-images.githubusercontent.com/251272/89673576-d4391100-d8a3-11ea-90a1-e946a0ca5b00.png)
   
   are there any rules for TXT_RECORD? if so, want to add it to your tooltip?
   
   ![image](https://user-images.githubusercontent.com/251272/89673532-c7b4b880-d8a3-11ea-9629-fca88d1ba1fe.png)
   


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467198001



##########
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:
       > also, there are 4 types, right?
   > 
   > ![image](https://user-images.githubusercontent.com/251272/89673576-d4391100-d8a3-11ea-90a1-e946a0ca5b00.png)
   > 
   > are there any rules for TXT_RECORD? if so, want to add it to your tooltip?
   > 
   > ![image](https://user-images.githubusercontent.com/251272/89673532-c7b4b880-d8a3-11ea-9629-fca88d1ba1fe.png)
   
   Code updated to reflect TXT_RECORD
   ![image](https://user-images.githubusercontent.com/22248619/89676459-ee292280-d8a8-11ea-92c8-d8ede1535845.png)
   
   




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467221772



##########
File path: traffic_ops/traffic_ops_golang/staticdnsentry/staticdnsentry.go
##########
@@ -103,6 +104,13 @@ 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
+		if len(address) != 0 {

Review comment:
       actually, why bother doing this if addressErr is not nil? so maybe
   
   `if addressErr == nil && len(address) != 0`




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467213114



##########
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:
       actually, even though it is required, it looks to me like it will still run this line of code no matter what:
   
   `
   lastChar := address[len(address)-1:]
   `




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4934: Validating absolute DNS name requirement on Static DNS Entry Type

Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4934:
URL: https://github.com/apache/trafficcontrol/pull/4934#discussion_r467222087



##########
File path: traffic_portal/app/src/common/modules/form/deliveryServiceStaticDnsEntry/form.deliveryServiceStaticDnsEntry.tpl.html
##########
@@ -57,9 +57,18 @@
                 </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.
+                        <br>4. Type:TXT_RECORD, address cannot be blank.
+                    </div>
+                </div>
+                </label>
                 <div class="col-md-10 col-sm-10 col-xs-12">
-                    <input id="address" name="address" type="text" class="form-control" ng-model="staticDnsEntry.address" required>
+                    <input id="address" name="address" type="text" class="form-control" ng-model="staticDnsEntry.address"
+                           required title= "Address must be: an IPv4, if type:A_RECORD; an IPv6, if type: AAAA_RECORD; end with a trailing period if type:CNAME_RECORD; cannot be blank; if type: TXT_RECORD">

Review comment:
       Yup a lot of them do.. It helps the user no matter where they are hovering: name, textbox
   ![image](https://user-images.githubusercontent.com/22248619/89679976-5844c600-d8af-11ea-8de9-c5767d7a9f21.png)
   




----------------------------------------------------------------
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:
users@infra.apache.org