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

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

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



##########
File path: traffic_ops/install/bin/_postinstall
##########
@@ -710,6 +721,30 @@ def unmarshal_config(dct): # type: (dict) -> dict[str, list[Question]]
 
 	return ret
 
+def write_encryption_key(aes_key_location): # type: (str) -> None
+	"""
+	Creates an AES encryption key for the postgres traffic vault backend
+
+	:param aes_key_location: Denotes the location of the aes encryption key file
+	:returns: None
+	"""
+	logging.info(aes_key_location)
+
+	cmd = ["/usr/bin/openssl", "rand", "-base64", "32"]
+	key = open(aes_key_location, 'w')

Review comment:
       File descriptor leak happens here if any of the following lines throw exceptions e.g. if `/usr/bin/openssl` does not exist or is not executable. This should either use context management like:
   ```python3
   with open(aes_key_location, "w") as key:
       # do stuff
   ```
   (not sure if ~~the dead version of Python nobody should ever use~~ Python2 supports that) or a `try`...`finally` like:
   ```python3
   key = open(aes_key_location, "w")
   try:
       # do stuff
   finally:
       key.close()
   ```

##########
File path: traffic_ops/install/bin/_postinstall
##########
@@ -905,7 +940,7 @@ def random_word(length = 12): # type: (int) -> str
 	word_chars = string.ascii_letters + string.digits + '_'
 	return ''.join(random.choice(word_chars) for _ in range(length))
 
-def generate_cdn_conf(questions, fname, automatic, root): # type: (list[Question], str, bool, str) -> None
+def generate_cdn_conf(questions, fname, automatic, root): # type: (list[Question], str, bool, str) -> bool
 	"""
 	Generates some properties of a cdn.conf file based on the passed questions.
 

Review comment:
       This doc string should specify what the function `:returns:`, now that it does.




-- 
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