Re: [Pki-devel] [PATCH] 278 - handle external certs

Review Notes: Looks like good stuff. Just a couple of small picky questions. We can delay the final ACK to after we either decide that the questions are not that important or we decide to make any minor changes. thanks, jack This method: in __init__.py : + def load_external_certs(self, conf_file): + # load external certs data + if os.path.exists(conf_file): + lines = open(conf_file).read().splitlines() + for line in lines: + parts = line.split('=', 1) + name = parts[0] + value = parts[1] + self.external_certs[name] = value + else: + self.external_certs['num_certs'] = 0 + I see if there is no file, the value of num_certs is 0. What about in the loop that reads the file when it does exist? What if it's empty? Also when doing the splitting for the values, do we need some simple sanity checking? Same Q's for method: def update_external_cert_conf(self, external_path, deployer): def delete_external_cert(self, nickname, token): + match_found = False + num_certs = int(self.external_certs['num_certs']) + if num_certs > 0: + for num in range(0, num_certs): + current_nick = self.external_certs[str(num) + ".nickname"] + current_token = self.external_certs[str(num) + ".token"] + if current_nick == nickname and current_token == token: + del self.external_certs[str(num) + ".nickname"] + del self.external_certs[str(num) + ".token"] + match_found = True + continue + if match_found: + self.external_certs[str(num - 1) + ".nickname"] = current_nick + self.external_certs[str(num - 1) + ".token"] = current_token + + self.external_certs['num_certs'] = num_certs - 1 + self.save_external_cert_data() I may have this wrong, but if you find a match and then continue? Will the "match_found if, ever get executed? Also you could if you wanted pre-calculate those hash indexes into a var and use them. Question: Here: nicks = self.import_certs( + instance, cert_file, nickname, token, trust_args) + self.update_instance_config(instance, nicks, token) + + self.print_message('Certificate imported for instance %s.' % + instance_name) Question is is there somewhere in that call chain that throws an exception if something goes wrong? Just checking to see if those late to lines get called (along with the print, even if there is an error importing certs) ?? Method: class InstanceExternalCertDeleteCLI(pki.cli.CLI): Would there be any value in allowing multiple nicknames to be deleted? Also same at above here: instance = pki.server.PKIInstance(instance_name) + instance.load() + + self.remove_cert(instance, nickname, token) + instance.delete_external_cert(nickname, token) + + self.print_message('Certificate removed from instance %s.' % + instance_name) Do we get to the end if instance.load does not work? ----- Original Message ----- From: "Ade Lee" <alee(a)redhat.com&gt; To: pki-devel(a)redhat.com Sent: Monday, February 29, 2016 9:25:30 AM Subject: [Pki-devel] [PATCH] 278 - handle external certs This is to resolve ticket 1742. For this ticket, we need a mechanism to import third party certs to clones. This patch provides a general mechanism to do this. A follow-on patch with documentation on how this all works is forthcoming. Ade _______________________________________________ Pki-devel mailing list Pki-devel(a)redhat.com https://www.redhat.com/mailman/listinfo/pki-devel