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
