On Wed, Apr 08, 2020 at 11:49:45AM +0200, Andrea Bolognani wrote:
On Tue, 2020-04-07 at 13:31 +0200, Erik Skultety wrote:
# The vault password is only needed for the jenkins flavor, but in # that case we want to make sure there's *something* in there - if self.get_flavor() != "test": + if self.get_flavor() == "jenkins": vault_pass_file = self._get_config_file("vault-password")
You need something similar to this in your new functions, otherwise trying even if you've configured lcitool to use the test flavor you'll still get
$ ./lcitool update all all ./lcitool: Missing or invalid gitlab url file ...
Doh! Clearly I didn't test regressions :(
+ def get_gitlab_runner_token_file(self): + gitlab_runner_token_file = self._get_config_file("gitlab-runner-token") + + try: + with open(gitlab_runner_token_file, "r") as infile: + if not infile.readline().strip(): + raise ValueError + except Exception as ex: + raise Exception( + "Missing or invalid gitlab runner token file ({}): {}".format(
Please capitalize GitLab properly in user-visible strings.
- extra_vars = json.dumps({ + extra_vars_d = {
You don't really need to change the name of the variable.
Right, although there are certain good practices like adding _l to lists and _d to dicts, so that it's immediately visible what kind of object you're working with when you don't have the initialization of the object right in front of your eyes.
+ if flavor == "gitlab": + extra_vars_d.update([ + ("gitlab_url_file", gitlab_url_file), + ("gitlab_runner_token_file", gitlab_runner_token_file), + ])
You can use a dict as argument to update(), which is more natural than a list of tuples.
^This is highly subjective I'd say, the readability doesn't really change if you don't count an extra pair of parentheses, but oki, I'll change it...
But in reality I think you can skip the check entirely and add both keys to extra_vars unconditionally: after you've fixed the issue I pointed out above, they will either be set (gitlab flavor) or None (any other flavor), and even in the latter case it's fine to pass them to Ansible as they will simply not be used.
I thought about this and I simply didn't want to pass undefined variables to ansible even if it doesn't use them, although I guess with the changes I'm currently working on it won't matter as I'm planning to stop passing visible extra vars on the command-line completely, so I think I can live with this suggestion. -- Erik Skultety