[libvirt-jenkins-ci PATCH v2 6/6] guests: lcitool: Enable the new 'gitlab' flavor in the lcitool script

[Erik Skultety]

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