Re: [libvirt-jenkins-ci PATCH v2 6/6] guests: lcitool: Enable the new 'gitlab' flavor in the lcitool script
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