[GitHub] asfgit commented on issue #1955: TO: in rpm build, set permissions of conf files to not be world-readable

[GitBox]

asfgit commented on issue #1955: TO: in rpm build, set permissions of conf 
files to not be world-readable
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1955#issuecomment-370061047
 
 
   
   Refer to this link for build results (access rights to CI server needed): 
   https://builds.apache.org/job/incubator-trafficcontrol-PR/1172/
   Test PASSed.
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] asfgit commented on issue #1950: Added CRUD for /parameters

[GitBox]

asfgit commented on issue #1950: Added CRUD for /parameters
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1950#issuecomment-370060328
 
 
   
   Refer to this link for build results (access rights to CI server needed): 
   https://builds.apache.org/job/incubator-trafficcontrol-PR/1171/
   Test PASSed.
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Jenkins build is back to normal : incubator-trafficcontrol-PR #1170

[Apache Jenkins Server]

See 

[GitHub] asfgit commented on issue #1956: Added CRUD for /types

[GitBox]

asfgit commented on issue #1956: Added CRUD for /types
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1956#issuecomment-370059586
 
 
   
   Refer to this link for build results (access rights to CI server needed): 
   https://builds.apache.org/job/incubator-trafficcontrol-PR/1170/
   Test PASSed.
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] asfgit commented on issue #1861: implement servers CRUD endpoints in go

[GitBox]

asfgit commented on issue #1861: implement servers CRUD endpoints in go
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1861#issuecomment-370058864
 
 
   
   Refer to this link for build results (access rights to CI server needed): 
   https://builds.apache.org/job/incubator-trafficcontrol-PR/1169/
   Test FAILed.
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] dangogh commented on issue #1955: TO: in rpm build, set permissions of conf files to not be world-readable

[GitBox]

dangogh commented on issue #1955: TO: in rpm build, set permissions of conf 
files to not be world-readable
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1955#issuecomment-370056509
 
 
   retest this please


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] dangogh commented on a change in pull request #1950: Added CRUD for /parameters

[GitBox]

dangogh commented on a change in pull request #1950: Added CRUD for /parameters
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1950#discussion_r171962424
 
 

 ##
 File path: traffic_ops/traffic_ops_golang/parameter/parameters.go
 ##
 @@ -0,0 +1,354 @@
+package parameter
+
+/*
+ * 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.
+ */
+
+import (
+   "errors"
+   "fmt"
+   "strconv"
+
+   "github.com/apache/incubator-trafficcontrol/lib/go-log"
+   "github.com/apache/incubator-trafficcontrol/lib/go-tc"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/api"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/auth"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers"
+
+   "github.com/jmoiron/sqlx"
+   "github.com/lib/pq"
+)
+
+//we need a type alias to define functions on
+type TOParameter tc.ParameterNullable
+
+//the refType is passed into the handlers where a copy of its type is used to 
decode the json.
+var refType = TOParameter(tc.ParameterNullable{})
+
+func GetRefType() *TOParameter {
+   return 
+}
+
+//Implementation of the Identifier, Validator interface functions
+func (parameter *TOParameter) GetID() (int, bool) {
+   if parameter.ID == nil {
+   return 0, false
+   }
+   return *parameter.ID, true
+}
+
+func (parameter *TOParameter) GetAuditName() string {
+   if parameter.Name != nil {
+   return *parameter.Name
+   }
+   if parameter.ID != nil {
+   return strconv.Itoa(*parameter.ID)
+   }
+   return "unknown"
+}
+
+func (parameter *TOParameter) GetType() string {
+   return "parameter"
+}
+
+func (parameter *TOParameter) SetID(i int) {
+   parameter.ID = 
+}
+
+func (pl *TOParameter) Validate(db *sqlx.DB) []error {
+   errs := []error{}
+   name := pl.Name
+   if name != nil && len(*name) < 1 {
+   errs = append(errs, errors.New(`Parameter 'name' is required.`))
+   }
+   return errs
+}
+
+//The TOParameter implementation of the Creator interface
+//all implementations of Creator should use transactions and return the proper 
errorType
+//ParsePQUniqueConstraintError is used to determine if a parameter with 
conflicting values exists
+//if so, it will return an errorType of DataConflict and the type should be 
appended to the
+//generic error message returned
+//The insert sql returns the id and lastUpdated values of the newly inserted 
parameter and have
+//to be added to the struct
+func (pl *TOParameter) Create(db *sqlx.DB, user auth.CurrentUser) (error, 
tc.ApiErrorType) {
+   rollbackTransaction := true
+   tx, err := db.Beginx()
+   defer func() {
+   if tx == nil || !rollbackTransaction {
+   return
+   }
+   err := tx.Rollback()
+   if err != nil {
+   log.Errorln(errors.New("rolling back transaction: " + 
err.Error()))
+   }
+   }()
+
+   if err != nil {
+   log.Error.Printf("could not begin transaction: %v", err)
+   return tc.DBError, tc.SystemError
+   }
+   resultRows, err := tx.NamedQuery(insertQuery(), pl)
+   if err != nil {
+   if pqErr, ok := err.(*pq.Error); ok {
+   err, eType := 
dbhelpers.ParsePQUniqueConstraintError(pqErr)
+   if eType == tc.DataConflictError {
+   return errors.New("a parameter with " + 
err.Error()), eType
+   }
+   return err, eType
+   }
+   log.Errorf("received non pq error: %++v from create execution", 
err)
+   return tc.DBError, tc.SystemError
+   }
+   defer resultRows.Close()
+
+   var id int
+   var lastUpdated tc.TimeNoMod
+   rowsAffected := 0
+   for resultRows.Next() {
+   rowsAffected++
+   if err := resultRows.Scan(, ); err != nil {
+   log.Error.Printf("could not scan id from insert: %s\n", 
err)
+   return tc.DBError, 

[GitHub] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

[GitBox]

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171951577
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/router/TrafficRouter.java
 ##
 @@ -675,17 +678,40 @@ public CacheLocation getCoverageZoneCacheLocation(final 
String ip, final String
return cacheLocation;
}
 
-   // We had a hit in the CZF but the name does not match a known 
cache location.
-   // Check whether the CZF entry has a geolocation and use it if 
so.
-   return 
getClosestCacheLocation(cacheRegister.filterAvailableLocations(deliveryServiceId),
 networkNode.getGeolocation(), 
cacheRegister.getDeliveryService(deliveryServiceId));
+   if (networkNode.getBackupLoc() != null) {
+   for (final String element : networkNode.getBackupLoc()) 
{
+   final CacheLocation bkCacheLocation = 
getCacheRegister().getCacheLocationById(element);
+   if (bkCacheLocation != null && 
!getSupportingCaches(bkCacheLocation.getCaches(), deliveryService).isEmpty()) {
+   LOGGER.debug("Got backup CZ cache group 
" + bkCacheLocation.getId() + " for " + ip + ", ds " + deliveryServiceId);
+   if (track != null) {
+   
track.setFromBackupCzGroup(true);
+   }
+   return bkCacheLocation;
+   }
+   }
+   } 
+
+   if (networkNode.isUseClosest()) {
+   // We had a hit in the CZF but the name does not match 
a known cache location.
+   // Check whether the CZF entry has a geolocation and 
use it if so.
+   final CacheLocation closestCacheLocation = 
getClosestCacheLocation(cacheRegister.filterAvailableLocations(deliveryServiceId),
 networkNode.getGeolocation(), 
cacheRegister.getDeliveryService(deliveryServiceId));
+   if (closestCacheLocation != null) {
+   LOGGER.debug("Got closest CZ cache group " + 
closestCacheLocation.getId() + " for " + ip + ", ds " + deliveryServiceId);
+   if (track != null) {
+   track.setFromBackupCzGroup(true);
 
 Review comment:
   in this case, it wasn't from backupCzGroup but the default behavior of 
nextClosest. Should we just omit this if-block?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

[GitBox]

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171956010
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/loc/NetworkNode.java
 ##
 @@ -106,15 +108,33 @@ public static NetworkNode generateTree(final JsonNode 
json, final boolean verify
 final String loc = czIter.next();
 final JsonNode locData = JsonUtils.getJsonNode(coverageZones, 
loc);
 final JsonNode coordinates = locData.get("coordinates");
+final JsonNode backupConfigNode = locData.get("backupZones");
+JsonNode fallbackNode = null; 
+boolean useClosestOnBackupFailure = false;
+
 Geolocation geolocation = null;
+List bkCacheLoc = null;
 
 if (coordinates != null && coordinates.has("latitude") && 
coordinates.has("longitude")) {
 final double latitude = 
coordinates.get("latitude").asDouble();
 final double longitude = 
coordinates.get("longitude").asDouble();
 geolocation = new Geolocation(latitude, longitude);
 }
 
-if (!addNetworkNodesToRoot(root, loc, locData, geolocation, 
useDeep)) {
+if (backupConfigNode != null) {
+if (backupConfigNode.has("list")) {
+bkCacheLoc = new ArrayList<>();
+for (final JsonNode network : 
JsonUtils.getJsonNode(backupConfigNode, "list")) {
+bkCacheLoc.add(network.asText());
+}
+}
+fallbackNode = 
backupConfigNode.get("fallbackToClosestGroup");
 
 Review comment:
   I think these two lines can be replaced with:
   ```
   useClosestOnBackupFailure = JsonUtils.optBoolean(backupConfigJson, 
"fallbackToClosestGroup", false);
   ```
   Then you also don't need the `fallbackNode` intermediate variable


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

[GitBox]

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171947866
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/loc/NetworkNode.java
 ##
 @@ -106,15 +108,33 @@ public static NetworkNode generateTree(final JsonNode 
json, final boolean verify
 final String loc = czIter.next();
 final JsonNode locData = JsonUtils.getJsonNode(coverageZones, 
loc);
 final JsonNode coordinates = locData.get("coordinates");
+final JsonNode backupConfigNode = locData.get("backupZones");
+JsonNode fallbackNode = null; 
+boolean useClosestOnBackupFailure = false;
+
 Geolocation geolocation = null;
+List bkCacheLoc = null;
 
 if (coordinates != null && coordinates.has("latitude") && 
coordinates.has("longitude")) {
 final double latitude = 
coordinates.get("latitude").asDouble();
 final double longitude = 
coordinates.get("longitude").asDouble();
 geolocation = new Geolocation(latitude, longitude);
 }
 
-if (!addNetworkNodesToRoot(root, loc, locData, geolocation, 
useDeep)) {
+if (backupConfigNode != null) {
+if (backupConfigNode.has("list")) {
+bkCacheLoc = new ArrayList<>();
+for (final JsonNode network : 
JsonUtils.getJsonNode(backupConfigNode, "list")) {
 
 Review comment:
   `cachegroup` rather than `network`?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

[GitBox]

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171949234
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/loc/NetworkNode.java
 ##
 @@ -198,12 +219,14 @@ public NetworkNode(final String str) throws 
NetworkNodeException {
 }
 
 public NetworkNode(final String str, final String loc) throws 
NetworkNodeException {
-this(str, loc, null);
+this(str, loc, null, null, false);
 }
 
-public NetworkNode(final String str, final String loc, final Geolocation 
geolocation) throws NetworkNodeException {
+public NetworkNode(final String str, final String loc, final Geolocation 
geolocation, final List backupCacheLoc, final boolean 
useClosestOnBackupFailure) throws NetworkNodeException {
 this.loc = loc;
 this.geolocation = geolocation;
+this.backupCacheLoc = backupCacheLoc;
 
 Review comment:
   `this.backupCacheGroups`?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

[GitBox]

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171925592
 
 

 ##
 File path: docs/source/admin/traffic_ops/using.rst
 ##
 @@ -881,7 +899,7 @@ The CZF is an input to the Traffic Control CDN, and as 
such does not get generat
 
 The script that generates the CZF file is not part of Traffic Control, since 
it is different for each situation.
 
-.. note:: The ``"coordinates"`` section is optional and may be used by Traffic 
Router for localization in the case of a CZF "hit" where the zone name does not 
map to a Cache Group name in Traffic Ops (i.e. Traffic Router will route to the 
closest Cache Group(s) geographically).
+.. note:: The ``"coordinates"`` section is optional and may be used by Traffic 
Router for localization in the case of a CZF "hit" where the zone name does not 
map to a Cache Group name in Traffic Ops (i.e. Traffic Router will route to the 
closest Cache Group(s) geographically). The ``"backupZones"`` section is 
optional and is used by Traffic Router for localization in the case of a CZF 
"hit" when there are no caches available for that DS in the matched cache 
group.  This backup list is ordered and if there are no caches available in any 
of the configured backup cache groups, the request will be bypassed (if 
configured) or rejected or gets to the closest cache groups if configured 
(``"fallbackToClosestGroup"`` flag) for this backup configuration.
 
 Review comment:
   I would break this last sentence up to make it clearer (starting from "This 
backup list...":
   This backup ``"list"`` contains an ordered list of backup groups to choose 
from if the matched cache group has no caches available for a requested DS. If 
an available cache cannot be found in any of the backup groups either, the 
``"fallbackToClosestGroup"`` flag determines the Traffic Router's following 
behavior. If ``true``, Traffic Router will find the next closest cache group 
with available caches. If ``false`` (the default), Traffic Router will bypass 
the request (if configured) or reject it.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

[GitBox]

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171948607
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/loc/NetworkNode.java
 ##
 @@ -106,15 +108,33 @@ public static NetworkNode generateTree(final JsonNode 
json, final boolean verify
 final String loc = czIter.next();
 final JsonNode locData = JsonUtils.getJsonNode(coverageZones, 
loc);
 final JsonNode coordinates = locData.get("coordinates");
+final JsonNode backupConfigNode = locData.get("backupZones");
+JsonNode fallbackNode = null; 
+boolean useClosestOnBackupFailure = false;
+
 Geolocation geolocation = null;
+List bkCacheLoc = null;
 
 if (coordinates != null && coordinates.has("latitude") && 
coordinates.has("longitude")) {
 final double latitude = 
coordinates.get("latitude").asDouble();
 final double longitude = 
coordinates.get("longitude").asDouble();
 geolocation = new Geolocation(latitude, longitude);
 }
 
-if (!addNetworkNodesToRoot(root, loc, locData, geolocation, 
useDeep)) {
+if (backupConfigNode != null) {
+if (backupConfigNode.has("list")) {
+bkCacheLoc = new ArrayList<>();
+for (final JsonNode network : 
JsonUtils.getJsonNode(backupConfigNode, "list")) {
+bkCacheLoc.add(network.asText());
+}
+}
+fallbackNode = 
backupConfigNode.get("fallbackToClosestGroup");
+useClosestOnBackupFailure = (fallbackNode != null) ? 
fallbackNode.asBoolean(): false;
+} else {
+useClosestOnBackupFailure = true;
+}
+
+if (!addNetworkNodesToRoot(root, loc, locData, 
geolocation,bkCacheLoc, useDeep, useClosestOnBackupFailure)) {
 
 Review comment:
   nit: space after `geolocation,`


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

[GitBox]

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171947182
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/loc/NetworkNode.java
 ##
 @@ -106,15 +108,33 @@ public static NetworkNode generateTree(final JsonNode 
json, final boolean verify
 final String loc = czIter.next();
 final JsonNode locData = JsonUtils.getJsonNode(coverageZones, 
loc);
 final JsonNode coordinates = locData.get("coordinates");
+final JsonNode backupConfigNode = locData.get("backupZones");
 
 Review comment:
   kind of a nitpick but can we call this variable `backupConfigJson`? All the 
`Node` stuff is confusing with JsonNode vs NetworkNode.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

[GitBox]

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171950963
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/router/TrafficRouter.java
 ##
 @@ -675,17 +678,40 @@ public CacheLocation getCoverageZoneCacheLocation(final 
String ip, final String
return cacheLocation;
}
 
-   // We had a hit in the CZF but the name does not match a known 
cache location.
-   // Check whether the CZF entry has a geolocation and use it if 
so.
-   return 
getClosestCacheLocation(cacheRegister.filterAvailableLocations(deliveryServiceId),
 networkNode.getGeolocation(), 
cacheRegister.getDeliveryService(deliveryServiceId));
+   if (networkNode.getBackupLoc() != null) {
+   for (final String element : networkNode.getBackupLoc()) 
{
 
 Review comment:
   `cachegroup` rather than `element`


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

[GitBox]

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171952777
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/loc/NetworkNode.java
 ##
 @@ -106,15 +108,33 @@ public static NetworkNode generateTree(final JsonNode 
json, final boolean verify
 final String loc = czIter.next();
 final JsonNode locData = JsonUtils.getJsonNode(coverageZones, 
loc);
 final JsonNode coordinates = locData.get("coordinates");
+final JsonNode backupConfigNode = locData.get("backupZones");
+JsonNode fallbackNode = null; 
+boolean useClosestOnBackupFailure = false;
+
 Geolocation geolocation = null;
+List bkCacheLoc = null;
 
 if (coordinates != null && coordinates.has("latitude") && 
coordinates.has("longitude")) {
 final double latitude = 
coordinates.get("latitude").asDouble();
 final double longitude = 
coordinates.get("longitude").asDouble();
 geolocation = new Geolocation(latitude, longitude);
 }
 
-if (!addNetworkNodesToRoot(root, loc, locData, geolocation, 
useDeep)) {
+if (backupConfigNode != null) {
+if (backupConfigNode.has("list")) {
+bkCacheLoc = new ArrayList<>();
+for (final JsonNode network : 
JsonUtils.getJsonNode(backupConfigNode, "list")) {
+bkCacheLoc.add(network.asText());
+}
+}
+fallbackNode = 
backupConfigNode.get("fallbackToClosestGroup");
+useClosestOnBackupFailure = (fallbackNode != null) ? 
fallbackNode.asBoolean(): false;
+} else {
+useClosestOnBackupFailure = true;
 
 Review comment:
   I think we should just initialize `useClosestOnBackupFailure` to true on 
L113 and omit this if-else block. It doesn't change behavior; it would just be 
cleaner because it will be set to false in the previous block if configured 
anyway.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

[GitBox]

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171950759
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/loc/NetworkNode.java
 ##
 @@ -281,6 +304,14 @@ public Geolocation getGeolocation() {
 return geolocation;
 }
 
+public List getBackupLoc() {
 
 Review comment:
   `getBackupCacheGroups`


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

[GitBox]

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171948090
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/loc/NetworkNode.java
 ##
 @@ -106,15 +108,33 @@ public static NetworkNode generateTree(final JsonNode 
json, final boolean verify
 final String loc = czIter.next();
 final JsonNode locData = JsonUtils.getJsonNode(coverageZones, 
loc);
 final JsonNode coordinates = locData.get("coordinates");
+final JsonNode backupConfigNode = locData.get("backupZones");
+JsonNode fallbackNode = null; 
+boolean useClosestOnBackupFailure = false;
+
 Geolocation geolocation = null;
+List bkCacheLoc = null;
 
 Review comment:
   `backupCacheGroups`?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

[GitBox]

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171947198
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/loc/NetworkNode.java
 ##
 @@ -106,15 +108,33 @@ public static NetworkNode generateTree(final JsonNode 
json, final boolean verify
 final String loc = czIter.next();
 final JsonNode locData = JsonUtils.getJsonNode(coverageZones, 
loc);
 final JsonNode coordinates = locData.get("coordinates");
+final JsonNode backupConfigNode = locData.get("backupZones");
+JsonNode fallbackNode = null; 
 
 Review comment:
   I think this variable is unneeded; see comment on L131


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rawlinp commented on a change in pull request #1908: Changes for Backup Edge Cache Group

[GitBox]

rawlinp commented on a change in pull request #1908: Changes for Backup Edge 
Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#discussion_r171950588
 
 

 ##
 File path: 
traffic_router/core/src/main/java/com/comcast/cdn/traffic_control/traffic_router/core/loc/NetworkNode.java
 ##
 @@ -137,18 +157,19 @@ public static NetworkNode generateTree(final JsonNode 
json, final boolean verify
 return null;
 }
 
+
+@SuppressWarnings({"PMD.CyclomaticComplexity", "PMD.NPathComplexity"})
 private static boolean addNetworkNodesToRoot(final SuperNode root, final 
String loc, final JsonNode locData,
- final Geolocation 
geolocation, final boolean useDeep) {
+ final Geolocation 
geolocation, final List bkCacheLoc, final boolean useDeep, final 
boolean useClosestOnBackupFailure) {
 
 Review comment:
   `backupCacheGroups` rather than `bkCacheLoc`?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] dewrich opened a new pull request #1956: Added CRUD for /types

[GitBox]

dewrich opened a new pull request #1956: Added CRUD for /types
URL: https://github.com/apache/incubator-trafficcontrol/pull/1956
 
 
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] asfgit commented on issue #1861: implement servers CRUD endpoints in go

[GitBox]

asfgit commented on issue #1861: implement servers CRUD endpoints in go
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1861#issuecomment-370044059
 
 
   
   Refer to this link for build results (access rights to CI server needed): 
   https://builds.apache.org/job/incubator-trafficcontrol-PR/1168/
   Test FAILed.
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Build failed in Jenkins: incubator-trafficcontrol-PR #1168

[Apache Jenkins Server]

See 


--
GitHub pull request #1861 of commit e9b8b9bad09c7475e191a00016be0d9a19212164, 
no merge conflicts.
Setting status of e9b8b9bad09c7475e191a00016be0d9a19212164 to PENDING with url 
https://builds.apache.org/job/incubator-trafficcontrol-PR/1168/ and message: 
'Build started sha1 is merged.'
Using context: default
[EnvInject] - Loading node environment variables.
Building remotely on H27 (ubuntu xenial) in workspace 

Cloning the remote Git repository
Cloning repository git://github.com/apache/incubator-trafficcontrol.git
 > git init  # 
 > timeout=10
Fetching upstream changes from 
git://github.com/apache/incubator-trafficcontrol.git
 > git --version # timeout=10
using GIT_SSH to set credentials 
 > git fetch --tags --progress 
 > git://github.com/apache/incubator-trafficcontrol.git 
 > +refs/heads/*:refs/remotes/origin/*
 > git config remote.origin.url 
 > git://github.com/apache/incubator-trafficcontrol.git # timeout=10
 > git config --add remote.origin.fetch +refs/heads/*:refs/remotes/origin/* # 
 > timeout=10
 > git config remote.origin.url 
 > git://github.com/apache/incubator-trafficcontrol.git # timeout=10
Fetching upstream changes from 
git://github.com/apache/incubator-trafficcontrol.git
using GIT_SSH to set credentials 
 > git fetch --tags --progress 
 > git://github.com/apache/incubator-trafficcontrol.git 
 > +refs/pull/*:refs/remotes/origin/pr/*
 > git rev-parse e9b8b9bad09c7475e191a00016be0d9a19212164^{commit} # timeout=10
 > git rev-parse origin/e9b8b9bad09c7475e191a00016be0d9a19212164^{commit} # 
 > timeout=10
 > git rev-parse e9b8b9bad09c7475e191a00016be0d9a19212164^{commit} # timeout=10
ERROR: Couldn't find any revision to build. Verify the repository and branch 
configuration for this job.
Retrying after 10 seconds
 > git rev-parse --is-inside-work-tree # timeout=10
Fetching changes from the remote Git repository
 > git config remote.origin.url 
 > git://github.com/apache/incubator-trafficcontrol.git # timeout=10
Fetching upstream changes from 
git://github.com/apache/incubator-trafficcontrol.git
 > git --version # timeout=10
using GIT_SSH to set credentials 
 > git fetch --tags --progress 
 > git://github.com/apache/incubator-trafficcontrol.git 
 > +refs/pull/*:refs/remotes/origin/pr/*
 > git rev-parse e9b8b9bad09c7475e191a00016be0d9a19212164^{commit} # timeout=10
 > git rev-parse origin/e9b8b9bad09c7475e191a00016be0d9a19212164^{commit} # 
 > timeout=10
 > git rev-parse e9b8b9bad09c7475e191a00016be0d9a19212164^{commit} # timeout=10
ERROR: Couldn't find any revision to build. Verify the repository and branch 
configuration for this job.
Retrying after 10 seconds
 > git rev-parse --is-inside-work-tree # timeout=10
Fetching changes from the remote Git repository
 > git config remote.origin.url 
 > git://github.com/apache/incubator-trafficcontrol.git # timeout=10
Fetching upstream changes from 
git://github.com/apache/incubator-trafficcontrol.git
 > git --version # timeout=10
using GIT_SSH to set credentials 
 > git fetch --tags --progress 
 > git://github.com/apache/incubator-trafficcontrol.git 
 > +refs/pull/*:refs/remotes/origin/pr/*
 > git rev-parse e9b8b9bad09c7475e191a00016be0d9a19212164^{commit} # timeout=10
 > git rev-parse origin/e9b8b9bad09c7475e191a00016be0d9a19212164^{commit} # 
 > timeout=10
 > git rev-parse e9b8b9bad09c7475e191a00016be0d9a19212164^{commit} # timeout=10
ERROR: Couldn't find any revision to build. Verify the repository and branch 
configuration for this job.
Archiving artifacts
Performing Post build task...
Match found for : : True
Logical operation result is TRUE
Running script  : export COMPOSE_HTTP_TIMEOUT=120
./bin/docker-compose -p "${BUILD_TAG}" -f 
infrastructure/docker/build/docker-compose.yml down -v
[incubator-trafficcontrol-PR] $ /bin/bash -xe /tmp/jenkins1702344525444763761.sh
+ export COMPOSE_HTTP_TIMEOUT=120
+ COMPOSE_HTTP_TIMEOUT=120
+ ./bin/docker-compose -p jenkins-incubator-trafficcontrol-PR-1168 -f 
infrastructure/docker/build/docker-compose.yml down -v
/tmp/jenkins1702344525444763761.sh: line 3: ./bin/docker-compose: No such file 
or directory
POST BUILD TASK : FAILURE
END OF POST BUILD TASK : 0

[GitHub] asfgit commented on issue #1955: TO: in rpm build, set permissions of conf files to not be world-readable

[GitBox]

asfgit commented on issue #1955: TO: in rpm build, set permissions of conf 
files to not be world-readable
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1955#issuecomment-370043890
 
 
   
   Refer to this link for build results (access rights to CI server needed): 
   https://builds.apache.org/job/incubator-trafficcontrol-PR/1167/
   Test FAILed.
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] dangogh commented on a change in pull request #1950: Added CRUD for /parameters

[GitBox]

dangogh commented on a change in pull request #1950: Added CRUD for /parameters
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1950#discussion_r171954361
 
 

 ##
 File path: traffic_ops/traffic_ops_golang/parameter/parameters.go
 ##
 @@ -0,0 +1,354 @@
+package parameter
+
+/*
+ * 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.
+ */
+
+import (
+   "errors"
+   "fmt"
+   "strconv"
+
+   "github.com/apache/incubator-trafficcontrol/lib/go-log"
+   "github.com/apache/incubator-trafficcontrol/lib/go-tc"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/api"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/auth"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers"
+
+   "github.com/jmoiron/sqlx"
+   "github.com/lib/pq"
+)
+
+//we need a type alias to define functions on
+type TOParameter tc.ParameterNullable
+
+//the refType is passed into the handlers where a copy of its type is used to 
decode the json.
+var refType = TOParameter(tc.ParameterNullable{})
+
+func GetRefType() *TOParameter {
+   return 
+}
+
+//Implementation of the Identifier, Validator interface functions
+func (parameter *TOParameter) GetID() (int, bool) {
+   if parameter.ID == nil {
+   return 0, false
+   }
+   return *parameter.ID, true
+}
+
+func (parameter *TOParameter) GetAuditName() string {
+   if parameter.Name != nil {
+   return *parameter.Name
+   }
+   if parameter.ID != nil {
+   return strconv.Itoa(*parameter.ID)
+   }
+   return "unknown"
+}
+
+func (parameter *TOParameter) GetType() string {
+   return "parameter"
+}
+
+func (parameter *TOParameter) SetID(i int) {
+   parameter.ID = 
+}
+
+func (pl *TOParameter) Validate(db *sqlx.DB) []error {
+   errs := []error{}
 
 Review comment:
   also affects json decoding -- nil comes out as null; empty slice as `[]`


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] alficles commented on a change in pull request #1950: Added CRUD for /parameters

[GitBox]

alficles commented on a change in pull request #1950: Added CRUD for /parameters
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1950#discussion_r171953955
 
 

 ##
 File path: traffic_ops/traffic_ops_golang/parameter/parameters.go
 ##
 @@ -0,0 +1,354 @@
+package parameter
+
+/*
+ * 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.
+ */
+
+import (
+   "errors"
+   "fmt"
+   "strconv"
+
+   "github.com/apache/incubator-trafficcontrol/lib/go-log"
+   "github.com/apache/incubator-trafficcontrol/lib/go-tc"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/api"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/auth"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers"
+
+   "github.com/jmoiron/sqlx"
+   "github.com/lib/pq"
+)
+
+//we need a type alias to define functions on
+type TOParameter tc.ParameterNullable
+
+//the refType is passed into the handlers where a copy of its type is used to 
decode the json.
+var refType = TOParameter(tc.ParameterNullable{})
+
+func GetRefType() *TOParameter {
+   return 
+}
+
+//Implementation of the Identifier, Validator interface functions
+func (parameter *TOParameter) GetID() (int, bool) {
+   if parameter.ID == nil {
+   return 0, false
+   }
+   return *parameter.ID, true
+}
+
+func (parameter *TOParameter) GetAuditName() string {
+   if parameter.Name != nil {
+   return *parameter.Name
+   }
+   if parameter.ID != nil {
+   return strconv.Itoa(*parameter.ID)
+   }
+   return "unknown"
+}
+
+func (parameter *TOParameter) GetType() string {
+   return "parameter"
+}
+
+func (parameter *TOParameter) SetID(i int) {
+   parameter.ID = 
+}
+
+func (pl *TOParameter) Validate(db *sqlx.DB) []error {
+   errs := []error{}
 
 Review comment:
   `errs := []error(nil)` would be my preferred answer. It's not meaningfully 
longer and it avoids an allocation with effectively no work. Still... the code 
as it exists is fine. It's mostly a note for future work.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] dewrich commented on a change in pull request #1950: Added CRUD for /parameters

[GitBox]

dewrich commented on a change in pull request #1950: Added CRUD for /parameters
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1950#discussion_r171952610
 
 

 ##
 File path: lib/go-tc/regions.go
 ##
 @@ -19,10 +19,12 @@ package tc
  * under the License.
  */
 
+// RegionsResponse ...
 
 Review comment:
   We plan on circling back to all the endpoints and document them with Swagger 
meta tags so those are temporary


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] dewrich commented on a change in pull request #1950: Added CRUD for /parameters

[GitBox]

dewrich commented on a change in pull request #1950: Added CRUD for /parameters
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1950#discussion_r171952225
 
 

 ##
 File path: traffic_ops/traffic_ops_golang/systeminfo/system_info.go
 ##
 @@ -87,15 +88,18 @@ WHERE p.config_file='global'`
 
info := make(map[string]string)
for rows.Next() {
-   p := tc.Parameter{}
+   p := tc.ParameterNullable{}
if err = rows.StructScan(); err != nil {
return nil, fmt.Errorf("getting system_info: %v", err)
}
-   if p.Secure && privLevel < auth.PrivLevelAdmin {
+   isSecure := *p.Secure
+   name := p.Name
+   value := p.Value
+   if isSecure && privLevel < auth.PrivLevelAdmin {
// Secure params only visible to admin
continue
}
-   info[p.Name] = p.Value
+   info[*name] = *value
 
 Review comment:
   Noted: will make changes accordingly


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rob05c commented on a change in pull request #1950: Added CRUD for /parameters

[GitBox]

rob05c commented on a change in pull request #1950: Added CRUD for /parameters
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1950#discussion_r171950527
 
 

 ##
 File path: traffic_ops/traffic_ops_golang/parameter/parameters.go
 ##
 @@ -0,0 +1,354 @@
+package parameter
+
+/*
+ * 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.
+ */
+
+import (
+   "errors"
+   "fmt"
+   "strconv"
+
+   "github.com/apache/incubator-trafficcontrol/lib/go-log"
+   "github.com/apache/incubator-trafficcontrol/lib/go-tc"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/api"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/auth"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers"
+
+   "github.com/jmoiron/sqlx"
+   "github.com/lib/pq"
+)
+
+//we need a type alias to define functions on
+type TOParameter tc.ParameterNullable
+
+//the refType is passed into the handlers where a copy of its type is used to 
decode the json.
+var refType = TOParameter(tc.ParameterNullable{})
+
+func GetRefType() *TOParameter {
+   return 
+}
+
+//Implementation of the Identifier, Validator interface functions
+func (parameter *TOParameter) GetID() (int, bool) {
+   if parameter.ID == nil {
+   return 0, false
+   }
+   return *parameter.ID, true
+}
+
+func (parameter *TOParameter) GetAuditName() string {
+   if parameter.Name != nil {
+   return *parameter.Name
+   }
+   if parameter.ID != nil {
+   return strconv.Itoa(*parameter.ID)
+   }
+   return "unknown"
+}
+
+func (parameter *TOParameter) GetType() string {
+   return "parameter"
+}
+
+func (parameter *TOParameter) SetID(i int) {
+   parameter.ID = 
+}
+
+func (pl *TOParameter) Validate(db *sqlx.DB) []error {
+   errs := []error{}
 
 Review comment:
   >var errs []error
   >it's a good practice to be in
   
   -1
   
   I use `errs := []error{}` not because it's an optimization, but because I 
think it's better to use a single way to create variables, i.e. `:=`. I'm not 
about to argue everyone should, and we should reject code with `var`; but I 
definitely prefer it, and I'd prefer we didn't demand people use `var` and use 
two different ways of creating variables.
   
   Also, if you feel strongly about the allocation, `errs := []error(nil)` 
works too. But I'd vote we don't require that either, unless performance 
matters.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rob05c commented on a change in pull request #1950: Added CRUD for /parameters

[GitBox]

rob05c commented on a change in pull request #1950: Added CRUD for /parameters
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1950#discussion_r171950527
 
 

 ##
 File path: traffic_ops/traffic_ops_golang/parameter/parameters.go
 ##
 @@ -0,0 +1,354 @@
+package parameter
+
+/*
+ * 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.
+ */
+
+import (
+   "errors"
+   "fmt"
+   "strconv"
+
+   "github.com/apache/incubator-trafficcontrol/lib/go-log"
+   "github.com/apache/incubator-trafficcontrol/lib/go-tc"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/api"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/auth"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers"
+
+   "github.com/jmoiron/sqlx"
+   "github.com/lib/pq"
+)
+
+//we need a type alias to define functions on
+type TOParameter tc.ParameterNullable
+
+//the refType is passed into the handlers where a copy of its type is used to 
decode the json.
+var refType = TOParameter(tc.ParameterNullable{})
+
+func GetRefType() *TOParameter {
+   return 
+}
+
+//Implementation of the Identifier, Validator interface functions
+func (parameter *TOParameter) GetID() (int, bool) {
+   if parameter.ID == nil {
+   return 0, false
+   }
+   return *parameter.ID, true
+}
+
+func (parameter *TOParameter) GetAuditName() string {
+   if parameter.Name != nil {
+   return *parameter.Name
+   }
+   if parameter.ID != nil {
+   return strconv.Itoa(*parameter.ID)
+   }
+   return "unknown"
+}
+
+func (parameter *TOParameter) GetType() string {
+   return "parameter"
+}
+
+func (parameter *TOParameter) SetID(i int) {
+   parameter.ID = 
+}
+
+func (pl *TOParameter) Validate(db *sqlx.DB) []error {
+   errs := []error{}
 
 Review comment:
   -1
   
   I use `errs := []error{}` not because it's an optimization, but because I 
think it's better to use a single way to create variables, i.e. `:=`. I'm not 
about to argue everyone should, and we should reject code with `var`; but I 
definitely prefer it, and I'd prefer we didn't demand people use `var` and use 
two different ways of creating variables.
   
   Also, if you feel strongly about the allocation, `errs := []error(nil)` 
works too. But I'd vote we don't require that either


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] dangogh commented on a change in pull request #1950: Added CRUD for /parameters

[GitBox]

dangogh commented on a change in pull request #1950: Added CRUD for /parameters
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1950#discussion_r171947924
 
 

 ##
 File path: traffic_ops/traffic_ops_golang/parameter/parameters.go
 ##
 @@ -0,0 +1,354 @@
+package parameter
+
+/*
+ * 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.
+ */
+
+import (
+   "errors"
+   "fmt"
+   "strconv"
+
+   "github.com/apache/incubator-trafficcontrol/lib/go-log"
+   "github.com/apache/incubator-trafficcontrol/lib/go-tc"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/api"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/auth"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers"
+
+   "github.com/jmoiron/sqlx"
+   "github.com/lib/pq"
+)
+
+//we need a type alias to define functions on
+type TOParameter tc.ParameterNullable
+
+//the refType is passed into the handlers where a copy of its type is used to 
decode the json.
+var refType = TOParameter(tc.ParameterNullable{})
+
+func GetRefType() *TOParameter {
+   return 
+}
+
+//Implementation of the Identifier, Validator interface functions
+func (parameter *TOParameter) GetID() (int, bool) {
+   if parameter.ID == nil {
 
 Review comment:
   and it's honestly not worth haggling over.   I've reviewed it and this is 
the only thing I brought up.   merging..


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] dangogh commented on a change in pull request #1950: Added CRUD for /parameters

[GitBox]

dangogh commented on a change in pull request #1950: Added CRUD for /parameters
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1950#discussion_r171947924
 
 

 ##
 File path: traffic_ops/traffic_ops_golang/parameter/parameters.go
 ##
 @@ -0,0 +1,354 @@
+package parameter
+
+/*
+ * 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.
+ */
+
+import (
+   "errors"
+   "fmt"
+   "strconv"
+
+   "github.com/apache/incubator-trafficcontrol/lib/go-log"
+   "github.com/apache/incubator-trafficcontrol/lib/go-tc"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/api"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/auth"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers"
+
+   "github.com/jmoiron/sqlx"
+   "github.com/lib/pq"
+)
+
+//we need a type alias to define functions on
+type TOParameter tc.ParameterNullable
+
+//the refType is passed into the handlers where a copy of its type is used to 
decode the json.
+var refType = TOParameter(tc.ParameterNullable{})
+
+func GetRefType() *TOParameter {
+   return 
+}
+
+//Implementation of the Identifier, Validator interface functions
+func (parameter *TOParameter) GetID() (int, bool) {
+   if parameter.ID == nil {
 
 Review comment:
   and it's honestly not worth haggling over.   I've reviewed it and this is 
the only thing I brought up.   merging..


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] alficles commented on a change in pull request #1950: Added CRUD for /parameters

[GitBox]

alficles commented on a change in pull request #1950: Added CRUD for /parameters
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1950#discussion_r171944478
 
 

 ##
 File path: traffic_ops/traffic_ops_golang/parameter/parameters.go
 ##
 @@ -0,0 +1,354 @@
+package parameter
+
+/*
+ * 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.
+ */
+
+import (
+   "errors"
+   "fmt"
+   "strconv"
+
+   "github.com/apache/incubator-trafficcontrol/lib/go-log"
+   "github.com/apache/incubator-trafficcontrol/lib/go-tc"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/api"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/auth"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers"
+
+   "github.com/jmoiron/sqlx"
+   "github.com/lib/pq"
+)
+
+//we need a type alias to define functions on
+type TOParameter tc.ParameterNullable
+
+//the refType is passed into the handlers where a copy of its type is used to 
decode the json.
+var refType = TOParameter(tc.ParameterNullable{})
+
+func GetRefType() *TOParameter {
+   return 
+}
+
+//Implementation of the Identifier, Validator interface functions
+func (parameter *TOParameter) GetID() (int, bool) {
+   if parameter.ID == nil {
+   return 0, false
+   }
+   return *parameter.ID, true
+}
+
+func (parameter *TOParameter) GetAuditName() string {
+   if parameter.Name != nil {
+   return *parameter.Name
+   }
+   if parameter.ID != nil {
+   return strconv.Itoa(*parameter.ID)
+   }
+   return "unknown"
+}
+
+func (parameter *TOParameter) GetType() string {
+   return "parameter"
+}
+
+func (parameter *TOParameter) SetID(i int) {
+   parameter.ID = 
+}
+
+func (pl *TOParameter) Validate(db *sqlx.DB) []error {
+   errs := []error{}
 
 Review comment:
   Do you intend to be allocating an empty array here? You can almost always 
save the allocation with `var errs []error`, so that in the typical case, that 
of no errors, it doesn't do any allocation?
   
   It's small and mostly pointless as an optimization in this particular code, 
but it's a good practice to be in. Also, this annoys some of the linters.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] alficles commented on a change in pull request #1950: Added CRUD for /parameters

[GitBox]

alficles commented on a change in pull request #1950: Added CRUD for /parameters
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1950#discussion_r171947162
 
 

 ##
 File path: traffic_ops/traffic_ops_golang/systeminfo/system_info.go
 ##
 @@ -87,15 +88,18 @@ WHERE p.config_file='global'`
 
info := make(map[string]string)
for rows.Next() {
-   p := tc.Parameter{}
+   p := tc.ParameterNullable{}
if err = rows.StructScan(); err != nil {
return nil, fmt.Errorf("getting system_info: %v", err)
}
-   if p.Secure && privLevel < auth.PrivLevelAdmin {
+   isSecure := *p.Secure
 
 Review comment:
   This will panic if p.Secure is null, which it will be if it's missing.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] alficles commented on a change in pull request #1950: Added CRUD for /parameters

[GitBox]

alficles commented on a change in pull request #1950: Added CRUD for /parameters
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1950#discussion_r171942670
 
 

 ##
 File path: lib/go-tc/regions.go
 ##
 @@ -19,10 +19,12 @@ package tc
  * under the License.
  */
 
+// RegionsResponse ...
 
 Review comment:
   These comments should generally be removed, I think. They suppress the 
linter but don't address its concern.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] alficles commented on a change in pull request #1950: Added CRUD for /parameters

[GitBox]

alficles commented on a change in pull request #1950: Added CRUD for /parameters
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1950#discussion_r171947506
 
 

 ##
 File path: traffic_ops/traffic_ops_golang/systeminfo/system_info.go
 ##
 @@ -87,15 +88,18 @@ WHERE p.config_file='global'`
 
info := make(map[string]string)
for rows.Next() {
-   p := tc.Parameter{}
+   p := tc.ParameterNullable{}
if err = rows.StructScan(); err != nil {
return nil, fmt.Errorf("getting system_info: %v", err)
}
-   if p.Secure && privLevel < auth.PrivLevelAdmin {
+   isSecure := *p.Secure
+   name := p.Name
+   value := p.Value
+   if isSecure && privLevel < auth.PrivLevelAdmin {
// Secure params only visible to admin
continue
}
-   info[p.Name] = p.Value
+   info[*name] = *value
 
 Review comment:
   This will panic if name or value are null, but they haven't been checked 
anywhere.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] dangogh commented on a change in pull request #1950: Added CRUD for /parameters

[GitBox]

dangogh commented on a change in pull request #1950: Added CRUD for /parameters
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1950#discussion_r171943098
 
 

 ##
 File path: traffic_ops/traffic_ops_golang/parameter/parameters.go
 ##
 @@ -0,0 +1,354 @@
+package parameter
+
+/*
+ * 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.
+ */
+
+import (
+   "errors"
+   "fmt"
+   "strconv"
+
+   "github.com/apache/incubator-trafficcontrol/lib/go-log"
+   "github.com/apache/incubator-trafficcontrol/lib/go-tc"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/api"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/auth"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers"
+
+   "github.com/jmoiron/sqlx"
+   "github.com/lib/pq"
+)
+
+//we need a type alias to define functions on
+type TOParameter tc.ParameterNullable
+
+//the refType is passed into the handlers where a copy of its type is used to 
decode the json.
+var refType = TOParameter(tc.ParameterNullable{})
+
+func GetRefType() *TOParameter {
+   return 
+}
+
+//Implementation of the Identifier, Validator interface functions
+func (parameter *TOParameter) GetID() (int, bool) {
+   if parameter.ID == nil {
 
 Review comment:
   yeah -- that was an over-generalization.   I agree that if the object is 
large that's a concern, but in these cases it's not.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] dangogh commented on issue #1904: TO 2.1 install error

[GitBox]

dangogh commented on issue #1904: TO 2.1 install error
URL: 
https://github.com/apache/incubator-trafficcontrol/issues/1904#issuecomment-370025034
 
 
   For further assistance,  I suggest joining the mailing list or slack channel 
-- you'll get much more immediate help there.  You can get info on those here:  
http://trafficcontrol.incubator.apache.org/ask/index.html


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] dangogh closed issue #1904: TO 2.1 install error

[GitBox]

dangogh closed issue #1904: TO 2.1 install error
URL: https://github.com/apache/incubator-trafficcontrol/issues/1904
 
 
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rob05c commented on a change in pull request #1950: Added CRUD for /parameters

[GitBox]

rob05c commented on a change in pull request #1950: Added CRUD for /parameters
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1950#discussion_r171916989
 
 

 ##
 File path: traffic_ops/traffic_ops_golang/parameter/parameters.go
 ##
 @@ -0,0 +1,354 @@
+package parameter
+
+/*
+ * 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.
+ */
+
+import (
+   "errors"
+   "fmt"
+   "strconv"
+
+   "github.com/apache/incubator-trafficcontrol/lib/go-log"
+   "github.com/apache/incubator-trafficcontrol/lib/go-tc"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/api"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/auth"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers"
+
+   "github.com/jmoiron/sqlx"
+   "github.com/lib/pq"
+)
+
+//we need a type alias to define functions on
+type TOParameter tc.ParameterNullable
+
+//the refType is passed into the handlers where a copy of its type is used to 
decode the json.
+var refType = TOParameter(tc.ParameterNullable{})
+
+func GetRefType() *TOParameter {
+   return 
+}
+
+//Implementation of the Identifier, Validator interface functions
+func (parameter *TOParameter) GetID() (int, bool) {
+   if parameter.ID == nil {
 
 Review comment:
   >these shouldn't be pointer receivers -- only methods that modify the 
content should be
   
   I don't agree. Non-pointers copy the entire object on every call. There's no 
reason to create an expensive copy of this object, just to get the ID.
   
   Reasons for value-functions are 
   * when the object is small, so the safety of being unable to modify the real 
object outweighs the performance cost
   * the function does modify the object for its own use, but it's necessary to 
leave the called object unmodified
   * The object is small enough, and the function called frequently enough, and 
performance matters, that a copy is faster than the cache-locality-breaking 
pointer lookup.
   * It's desirable to avoid fulfilling a pointer interface
   
   None of those apply here.
   
   That said, the object isn't _that_ big, I don't feel super-strongly that it 
needs to be a pointer, if you feel that strongly in the other direction.
   
   But I disagree strongly with the statement "only methods that modify the 
content should be [pointer receivers]". Value member functions in Go create a 
copy of the entire object on every call, and that needs to be considered.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] dangogh closed pull request #1953: TO golang -- adds validation to regions

[GitBox]

dangogh closed pull request #1953: TO golang -- adds validation to regions
URL: https://github.com/apache/incubator-trafficcontrol/pull/1953
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/lib/go-tc/regions.go b/lib/go-tc/regions.go
index 379d56e3f9..bf4e72a85f 100644
--- a/lib/go-tc/regions.go
+++ b/lib/go-tc/regions.go
@@ -30,3 +30,11 @@ type Region struct {
LastUpdated  TimeNoMod `json:"lastUpdated" db:"last_updated"`
Name string`json:"name" db:"name"`
 }
+
+type RegionNullable struct {
+   DivisionName *string`json:"divisionName"`
+   Division *int   `json:"division" db:"division"`
+   ID   *int   `json:"id" db:"id"`
+   LastUpdated  *TimeNoMod `json:"lastUpdated" db:"last_updated"`
+   Name *string`json:"name" db:"name"`
+}
diff --git a/traffic_ops/traffic_ops_golang/region/regions.go 
b/traffic_ops/traffic_ops_golang/region/regions.go
index 6719f3e2e1..c77065d4df 100644
--- a/traffic_ops/traffic_ops_golang/region/regions.go
+++ b/traffic_ops/traffic_ops_golang/region/regions.go
@@ -22,49 +22,59 @@ package region
 import (
"errors"
"fmt"
+   "strconv"
 
"github.com/apache/incubator-trafficcontrol/lib/go-log"
"github.com/apache/incubator-trafficcontrol/lib/go-tc"

"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/api"

"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/auth"

"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/tovalidate"
+   validation "github.com/go-ozzo/ozzo-validation"
"github.com/jmoiron/sqlx"
"github.com/lib/pq"
 )
 
 //we need a type alias to define functions on
-type TORegion tc.Region
+type TORegion tc.RegionNullable
 
 //the refType is passed into the handlers where a copy of its type is used to 
decode the json.
-var refType = TORegion(tc.Region{})
+var refType = TORegion(tc.RegionNullable{})
 
 func GetRefType() *TORegion {
return 
 }
 
 //Implementation of the Identifier, Validator interface functions
-func (region *TORegion) GetID() (int, bool) {
-   return region.ID, true
+func (region TORegion) GetID() (int, bool) {
+   if region.ID == nil {
+   return 0, false
+   }
+   return *region.ID, true
 }
 
-func (region *TORegion) GetAuditName() string {
-   return region.Name
+func (region TORegion) GetAuditName() string {
+   if region.Name == nil {
+   id, _ := region.GetID()
+   return strconv.Itoa(id)
+   }
+   return *region.Name
 }
 
-func (region *TORegion) GetType() string {
+func (region TORegion) GetType() string {
return "region"
 }
 
 func (region *TORegion) SetID(i int) {
-   region.ID = i
+   region.ID = 
 }
 
-func (region *TORegion) Validate(db *sqlx.DB) []error {
-   errs := []error{}
-   if len(region.Name) < 1 {
-   errs = append(errs, errors.New(`Region 'name' is required.`))
+func (region TORegion) Validate(db *sqlx.DB) []error {
+   errs := validation.Errors{
+   "name":   validation.Validate(region.Name, 
validation.Required),
+   "divisionId": validation.Validate(region.Division, 
validation.NotNil, validation.Min(0)),
}
-   return errs
+   return tovalidate.ToErrors(errs)
 }
 
 func (region *TORegion) Read(db *sqlx.DB, parameters map[string]string, user 
auth.CurrentUser) ([]interface{}, []error, tc.ApiErrorType) {
@@ -94,7 +104,7 @@ func (region *TORegion) Read(db *sqlx.DB, parameters 
map[string]string, user aut
 
regions := []interface{}{}
for rows.Next() {
-   var s tc.Region
+   var s TORegion
if err = rows.StructScan(); err != nil {
log.Errorf("error parsing Region rows: %v", err)
return nil, []error{tc.DBError}, tc.SystemError
@@ -165,7 +175,7 @@ func (region *TORegion) Update(db *sqlx.DB, user 
auth.CurrentUser) (error, tc.Ap
}
}
log.Debugf("lastUpdated: %++v", lastUpdated)
-   region.LastUpdated = lastUpdated
+   region.LastUpdated = 
if rowsAffected != 1 {
if rowsAffected < 1 {
return errors.New("no region found with this id"), 
tc.DataMissingError
@@ -240,7 +250,7 @@ func (region *TORegion) Create(db *sqlx.DB, user 
auth.CurrentUser) (error, tc.Ap
return tc.DBError, tc.SystemError
}
region.SetID(id)
-   region.LastUpdated = lastUpdated
+   region.LastUpdated = 
err = tx.Commit()
   

[GitHub] dangogh commented on a change in pull request #1950: Added CRUD for /parameters

[GitBox]

dangogh commented on a change in pull request #1950: Added CRUD for /parameters
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1950#discussion_r171911314
 
 

 ##
 File path: traffic_ops/traffic_ops_golang/parameter/parameters.go
 ##
 @@ -0,0 +1,354 @@
+package parameter
+
+/*
+ * 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.
+ */
+
+import (
+   "errors"
+   "fmt"
+   "strconv"
+
+   "github.com/apache/incubator-trafficcontrol/lib/go-log"
+   "github.com/apache/incubator-trafficcontrol/lib/go-tc"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/api"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/auth"
+   
"github.com/apache/incubator-trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers"
+
+   "github.com/jmoiron/sqlx"
+   "github.com/lib/pq"
+)
+
+//we need a type alias to define functions on
+type TOParameter tc.ParameterNullable
+
+//the refType is passed into the handlers where a copy of its type is used to 
decode the json.
+var refType = TOParameter(tc.ParameterNullable{})
+
+func GetRefType() *TOParameter {
+   return 
+}
+
+//Implementation of the Identifier, Validator interface functions
+func (parameter *TOParameter) GetID() (int, bool) {
+   if parameter.ID == nil {
 
 Review comment:
   these shouldn't be pointer receivers -- only methods that modify the content 
should be,  e.g. SetID()..:
   ```
   func (parameter TOParameter) GetID() ...
   ```


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] asfgit commented on issue #1950: Added CRUD for /parameters

[GitBox]

asfgit commented on issue #1950: Added CRUD for /parameters
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1950#issuecomment-369984274
 
 
   
   Refer to this link for build results (access rights to CI server needed): 
   https://builds.apache.org/job/incubator-trafficcontrol-PR/1164/
   Test PASSed.
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] dangogh opened a new pull request #1953: TO golang -- adds validation to regions

[GitBox]

dangogh opened a new pull request #1953: TO golang -- adds validation to regions
URL: https://github.com/apache/incubator-trafficcontrol/pull/1953
 
 
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] asfgit commented on issue #1950: Added CRUD for /parameters

[GitBox]

asfgit commented on issue #1950: Added CRUD for /parameters
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1950#issuecomment-369982471
 
 
   
   Refer to this link for build results (access rights to CI server needed): 
   https://builds.apache.org/job/incubator-trafficcontrol-PR/1163/
   Test PASSed.
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] asfgit commented on issue #1952: TO golang -- validation checks for statuses

[GitBox]

asfgit commented on issue #1952: TO golang -- validation checks for statuses
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1952#issuecomment-369977636
 
 
   
   Refer to this link for build results (access rights to CI server needed): 
   https://builds.apache.org/job/incubator-trafficcontrol-PR/1162/
   Test PASSed.
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] dangogh opened a new pull request #1952: TO golang -- validation checks for statuses

[GitBox]

dangogh opened a new pull request #1952: TO golang -- validation checks for 
statuses
URL: https://github.com/apache/incubator-trafficcontrol/pull/1952
 
 
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] asfgit commented on issue #1951: added routes for PUT/DELETE to /divisions

[GitBox]

asfgit commented on issue #1951: added routes for PUT/DELETE to /divisions
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1951#issuecomment-369972903
 
 
   
   Refer to this link for build results (access rights to CI server needed): 
   https://builds.apache.org/job/incubator-trafficcontrol-PR/1161/
   Test PASSed.
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] asfgit commented on issue #1950: Added CRUD for /parameters

[GitBox]

asfgit commented on issue #1950: Added CRUD for /parameters
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1950#issuecomment-369971670
 
 
   
   Refer to this link for build results (access rights to CI server needed): 
   https://builds.apache.org/job/incubator-trafficcontrol-PR/1160/
   Test PASSed.
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] dewrich opened a new pull request #1951: added routes for PUT/DELETE to /divisions

[GitBox]

dewrich opened a new pull request #1951: added routes for PUT/DELETE to 
/divisions
URL: https://github.com/apache/incubator-trafficcontrol/pull/1951
 
 
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] dewrich opened a new pull request #1950: Add PUT/DELETE route definitions for /divisions

[GitBox]

dewrich opened a new pull request #1950: Add PUT/DELETE route definitions for 
/divisions
URL: https://github.com/apache/incubator-trafficcontrol/pull/1950
 
 
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] asfgit commented on issue #1924: Implemented API tests for new routes

[GitBox]

asfgit commented on issue #1924: Implemented API tests for new routes
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1924#issuecomment-369958874
 
 
   
   Refer to this link for build results (access rights to CI server needed): 
   https://builds.apache.org/job/incubator-trafficcontrol-PR/1159/
   Test PASSed.
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Jenkins build is back to normal : incubator-trafficcontrol-PR #1159

[Apache Jenkins Server]

See 

[GitHub] asfgit commented on issue #1799: Traffic Ops API/GUI configuration of FQ Pacing plugin

[GitBox]

asfgit commented on issue #1799: Traffic Ops API/GUI configuration of FQ Pacing 
plugin
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1799#issuecomment-369945594
 
 
   
   Refer to this link for build results (access rights to CI server needed): 
   https://builds.apache.org/job/incubator-trafficcontrol-PR/1158/
   Test FAILed.
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] limited commented on issue #1799: Traffic Ops API/GUI configuration of FQ Pacing plugin

[GitBox]

limited commented on issue #1799: Traffic Ops API/GUI configuration of FQ 
Pacing plugin
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1799#issuecomment-369929016
 
 
   test this please
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] limited commented on issue #1908: Changes for Backup Edge Cache Group

[GitBox]

limited commented on issue #1908: Changes for Backup Edge Cache Group
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1908#issuecomment-369922600
 
 
   I'm good with this. 
   
   @rawlinp Any more comments?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] asfgit commented on issue #1799: Traffic Ops API/GUI configuration of FQ Pacing plugin

[GitBox]

asfgit commented on issue #1799: Traffic Ops API/GUI configuration of FQ Pacing 
plugin
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1799#issuecomment-369921120
 
 
   
   Refer to this link for build results (access rights to CI server needed): 
   https://builds.apache.org/job/incubator-trafficcontrol-PR/1157/
   Test FAILed.
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Build failed in Jenkins: incubator-trafficcontrol-PR #1157

[Apache Jenkins Server]

See 


Changes:

[jeffrey_elsloo] added geolocationOverrides to CRConfig and TR

[jeffrey_elsloo] updated CRConfig param to use camelcase

[jeffrey_elsloo] changed param name to maxmind.default.override

[jeffrey_elsloo] documentation for maxmind.default.override parameter

[jeffrey_elsloo] modified crconfig diff for maxmindDefaultOverride param

[jeffrey_elsloo] code cleanup

[jeffrey_elsloo] renamed isDefault to defaultLocation

[jeffrey_elsloo] Updated configuration.rst and renamed defaultGeolocations

[dangogh] converted the read to use the Read() interface

[dangogh] renamed the Inserter interface to Creator to match the CRUD acronym

[dangogh] fixed test cases for the Inserter to Creator interface name change

[dangogh] updated comment to reflect the Creator refactor

[dangogh] added functionality to support CRUD on the /divisions endpoint

[dangogh] added TO Client funcs for invoking the /divisions CRUD

[dangogh] cleaned up the cdns to account for the cdn package

[dangogh] refactored the asns to refer to the CRUD interface framework

[dangogh] cleaned up queries to account for the package names

[dangogh] added TO client for ASNs

[dangogh] added more test fixtures

[dangogh] updated to only deleteByID

[dangogh] fixed asns test case

[dangogh] added the TestInterface

[dangogh] added TestInterfaces test

[dangogh] added TestInterfaces test

[dangogh] updated to support the GetID() interface change and the Inserter to

[dangogh] fixed merge conflict

[dangogh] fixed merge issue

[dangogh] fixed merge issue

[dangogh] ds Inserter->Creator

[dangogh] fix sql syntax

[dangogh] fix column names; include cachegroup table in queries

[dewrich] updates traffic portal install instructions and changes the defaults 
of

[dangogh] updated to use the Read() interface

[dangogh] added statuses test data

[dangogh] updated to implement the CUD for statuses

[dangogh] removed print

[dangogh] cleaned up test case

[dangogh] added TestInterfaces test

[dangogh] fixed GetID() interface change

[dangogh] updated for the Inserter->Creator interface change

[mitchell852] fix expected msg

[mitchell852] fix status update query

[dewrich] fix ds request reader.  if empty list, return empty slice rather than

[dangogh] updated to use the CRUD interface

[dangogh] updated regions to use the CRUD interface

[dangogh] fix the insert statement

[dangogh] added API tests for /regions

[dangogh] cleaned up test case

[dangogh] added TestInterfaces test

[dangogh] fixed GetID() interface change

[dangogh] cleanup; fix column mappings

[dangogh] fixed test case to use Creator instead of Inserter

[dangogh] fixed to use Creator instead of Inserter

[dangogh] prevents parent.config failure when no delivery services assinged

[rob05c] extra flag needed for go build in rpm

[dangogh] moved into systeminfo package

[dangogh] rebase cleanup

[dangogh] adds stronger confirm when creating a request to delete a ds

[neuman] requires portal role to manage delivery service steering targets

[dangogh] updated phys_locations to use the CRUD interface

[dangogh] aligned the query parameters

[dangogh] updated for the API test cases

[dangogh] added TestInterfaces test

[dangogh] remapped region and regionId

[dangogh] fixed the region name and region id confusion

[dangogh] modified the expected name

[dangogh] fixed GetID() interface change

[dangogh] cleaned up physlocations to function with api calls

[dangogh] Updates based upon review comments

[dangogh] reference the nullable for table reading

[dangogh] fixed compiler error with import alias

[dangogh] updated comment

[dangogh] removed duplicate routes

[dangogh] null for null db data; region instead of regionName

[dangogh] db sees region_id as region

[dangogh] backup plan for GetAuditName

[dangogh] remove the improper use of the NoAuth flag disabling user 
authentication

[neuman] Adding Qwilt to the list of TC users

[dewrich] managing sslkeys and urlsig keys requires at least the operations role

[friede] Add weasel to the docker build.

[dangogh] adds manage ssl keys, url sig keys and uri signing keys menu items to 
TP

[dewrich] move config.go into its own package

[dangogh] Revert "move config.go into its own package"

[dangogh] ds requests ui tweaks - removed delay on ds form tooltips; made sure

[dewrich] add Logger interface

[dewrich] add custom Logger method

[dewrich] rename to ChangeLogger

[dewrich] make message more clear

[dewrich] remove db from ChangeLogger method

[dewrich] fixing changelog message for deliveryservice_request assignment and

[dewrich] pull data from db after assign/status change; fix delete message issue

[dangogh] TP ds forms pull field titles/descriptions from

[friede] Traffic Ops API/GUI and Traffic Portal configuration of FQ Pacing 
plugin

--
[...truncated 3.10 KB...]
 Built:Thu May  4 22:10:54 2017

[GitHub] limited commented on a change in pull request #1799: Traffic Ops API/GUI configuration of FQ Pacing plugin

[GitBox]

limited commented on a change in pull request #1799: Traffic Ops API/GUI 
configuration of FQ Pacing plugin
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1799#discussion_r171846377
 
 

 ##
 File path: traffic_portal/app/src/traffic_portal_properties.json
 ##
 @@ -199,6 +199,10 @@
 "title": "Global Max TPS",
 "desc": "The maximum transactions this delivery service can serve 
across all EDGE caches before traffic will be diverted to the bypass 
destination. For a DNS delivery service, the Bypass Ipv4 or Ipv6 will be used 
(depending on whether this was a A or  request), and for HTTP delivery 
services the Bypass FQDN will be used."
   },
+ "fqPacingRate": {
+   "title": "Fair Queuing Pacing Rate Bps",
+"desc": "The maximum bytes per second a cache will delivery on any 
single TCP connection. This uses the Linux kernel's Fair Queuing 
setsockopt(SO_MAX_PACING_RATE) to limit the rate of delivery. Traffic exceeding 
this speed will only be rate-limited and not diverted. This option requires 
'net.core.default_qdisc = fq' in /etc/sysctl.conf"
 
 Review comment:
   removed that sentence
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] limited commented on a change in pull request #1799: Traffic Ops API/GUI configuration of FQ Pacing plugin

[GitBox]

limited commented on a change in pull request #1799: Traffic Ops API/GUI 
configuration of FQ Pacing plugin
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1799#discussion_r171845625
 
 

 ##
 File path: 
traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.DNS.tpl.html
 ##
 @@ -468,6 +468,18 @@
 
 
 
+
+
+{{label('fqPacingRate', 'title')}}
 
 Review comment:
   done


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] limited commented on a change in pull request #1799: Traffic Ops API/GUI configuration of FQ Pacing plugin

[GitBox]

limited commented on a change in pull request #1799: Traffic Ops API/GUI 
configuration of FQ Pacing plugin
URL: 
https://github.com/apache/incubator-trafficcontrol/pull/1799#discussion_r171845600
 
 

 ##
 File path: 
traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.HTTP.tpl.html
 ##
 @@ -471,6 +471,18 @@
 
 
 
+   
+
+{{label('fqPacingRate', 'title')}}
 
 Review comment:
   done


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services