[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5857: CIAB: Use postgres as the default traffic vault backend

[GitBox <gi...@apache.org>]

rawlinp commented on a change in pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#discussion_r633891448



##########
File path: traffic_ops/install/bin/_postinstall
##########
@@ -229,10 +231,22 @@ DEFAULTS = {
 		Question("Traffic Ops database user", "traffic_ops", "user"),
 		Question("Password for Traffic Ops database user", "", "password", hidden=True)
 	],
+	TV_DATABASE_CONF_FILE: [

Review comment:
       this is confusing to me since these options actually live in `cdn.conf` -- shouldn't the questions be for that instead?

##########
File path: traffic_ops/app/conf/production/tv.conf
##########
@@ -0,0 +1,11 @@
+

Review comment:
       if this file is just for CiaB configuration we should probably keep it in the `cdn-in-a-box` directory

##########
File path: traffic_ops/app/conf/cdn.conf
##########
@@ -10,6 +10,19 @@
         "workers" : 12
     },
     "traffic_ops_golang" : {
+        "traffic_vault_backend": "postgres",

Review comment:
       Did you mean to commit these config changes? The default config should probably leave TV not configured since it's an optional component. 

##########
File path: infrastructure/cdn-in-a-box/traffic_ops/run-go.sh
##########
@@ -138,11 +143,12 @@ cd /opt/traffic_ops/app;
 CDNCONF=/opt/traffic_ops/app/conf/cdn.conf
 DBCONF=/opt/traffic_ops/app/conf/production/database.conf
 RIAKCONF=/opt/traffic_ops/app/conf/production/riak.conf
+TV_DBCONF=/opt/traffic_ops/app/conf/production/tv.conf

Review comment:
       this doesn't seem to be used anywhere

##########
File path: traffic_ops/install/bin/_postinstall
##########
@@ -1004,6 +1018,14 @@ def generate_cdn_conf(questions, fname, automatic, root): # type: (list[Question
 		json.dump(existing_conf, conf_file, indent=indent)
 		print(file=conf_file)
 	logging.info("CDN configuration has been saved")
+	try:

Review comment:
       a better way to do this would be:
   ```
   return existing_conf["traffic_ops_golang"].get("traffic_vault_backend") == "postgres"
   ```
   but since `traffic_vault_backend` is not a required field, should we actually be raising an exception in this case?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org