On Fri, Aug 31, 2018 at 12:16:14PM +0200, Andrea Bolognani wrote:
> On Wed, 2018-08-29 at 17:08 +0200, Andrea Bolognani wrote:
> > This will allow users to build arbitrary branches from
> > arbitrary git repositories, but for the moment all it
> > does is parse the argument and pass it down to the
> > Ansible playbook.
> >
> > Signed-off-by: Andrea Bolognani <abologna@xxxxxxxxxx>
> > ---
> > guests/lcitool | 36 ++++++++++++++++++++++++++----------
> > 1 file changed, 26 insertions(+), 10 deletions(-)
>
> As helpfully pointed out by Erik, this change has the unintended
> consequence of making '-r REVISION' mandatory every time a playbook
> is invoked, including when using '-a update'.
>
> The patch below fixes the issue; consider it squashed in.
>
> diff --git a/guests/lcitool b/guests/lcitool
> index c12143d..0729d55 100755
> --- a/guests/lcitool
> +++ b/guests/lcitool
> @@ -367,13 +367,15 @@ class Application:
> ansible_hosts = ",".join(self._inventory.expand_pattern(hosts))
> selected_projects = self._projects.expand_pattern(projects)
>
> - if revision is None:
> - raise Error("Missing git revision")
> - tokens = revision.split('/')
> - if len(tokens) < 2:
> - raise Error("Invalid git revision '{}'".format(revision))
> - git_remote = tokens[0]
> - git_branch = '/'.join(tokens[1:])
> + if revision is not None:
> + tokens = revision.split('/')
> + if len(tokens) < 2:
> + raise Error("Invalid git revision '{}'".format(revision))
> + git_remote = tokens[0]
> + git_branch = '/'.join(tokens[1:])
> + else:
> + git_remote = None
> + git_branch = None
Sorry for the delay. ^This should rather be:
git_remote = "upstream"
git_branch = "master"
... and the check below should go away, I still don't think we should mandate
specifying revisions at all times.
Initially, I wanted to point out that "revision" is probably not the best name
given the information in man gitrevisions(7) and I wanted to suggest "refname"
instead, but then I tried referencing both with <sha1> as well as
<describeOutput> and it worked as expected, sweet. I also tried referencing
with @{<n>}, however ansible documents that kind of limitation.
>
> ansible_cfg_path = os.path.join(base, "ansible.cfg")
> playbook_base = os.path.join(base, "playbooks", playbook)
> @@ -494,6 +496,9 @@ class Application:
> self._execute_playbook("update", hosts, projects, revision)
>
> def _action_build(self, hosts, projects, revision):
> + if revision is None:
> + raise Error("Missing git revision")
see above...as I said, I still don't see a reason for requiring ^this, if there
truly is a compelling reason to keep it, then I'd suggest changing the default
remote name to "origin" from "upstream", because it feels more natural and
intuitive to me. Even though you document this in the README and I understand
the reasoning - you can name your origin whatever you want + you can clone your
fork and then add an upstream remote, but I wouldn't expect that to be very
common practice.
Other than that, the patch works.
Erik
> +
> self._execute_playbook("build", hosts, projects, revision)
>
> def _action_dockerfile(self, hosts, projects, _revision):
> --
> Andrea Bolognani / Red Hat / Virtualization
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
