Re: [libvirt] [jenkins-ci PATCH 1/8] lcitool: Add "-r REVISION" argument for build
On Tue, Sep 04, 2018 at 10:33:18AM +0200, Andrea Bolognani wrote:
> On Tue, 2018-09-04 at 09:55 +0200, Erik Skultety wrote:
> > Initially, I wanted to point out that "revision" is probably not the best
> > name
> [...]
>
> I was thinking yesterday that perhaps it would be better to
> use '-g GITREVISION' here instead, to leave '-r' available for
> future extensions, eg. when/if I get around to adding support
> for dependencies between projects that could be used to mean
> 'recursive', as in: build this one project and also all those
> that depend on it, the same way CentOS CI does it. Does that
> sound reasonable?
Yep, I've been playing with the same thought too, so '-g' sounds fine to me.
>
> [...]
> > > 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.
>
> Let's just use "default", that one doesn't have any charged
> meaning in the git world and it's pretty accurate, too.
Fair enough.
>
> I'll also make the whole thing optional. I still have a slightly
> uncomfortable feeling about it, though I can't quite put it to
> words... Plus unlike with libvirt going one way or another won't
> quite lock us into that decision forever, so I'm more willing to
> just try stuff ;)
Reviewed-by: Erik Skultety
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH 1/8] lcitool: Add "-r REVISION" argument for build
On Tue, 2018-09-04 at 09:55 +0200, Erik Skultety wrote:
> Initially, I wanted to point out that "revision" is probably not the best name
[...]
I was thinking yesterday that perhaps it would be better to
use '-g GITREVISION' here instead, to leave '-r' available for
future extensions, eg. when/if I get around to adding support
for dependencies between projects that could be used to mean
'recursive', as in: build this one project and also all those
that depend on it, the same way CentOS CI does it. Does that
sound reasonable?
[...]
> > 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.
Let's just use "default", that one doesn't have any charged
meaning in the git world and it's pretty accurate, too.
I'll also make the whole thing optional. I still have a slightly
uncomfortable feeling about it, though I can't quite put it to
words... Plus unlike with libvirt going one way or another won't
quite lock us into that decision forever, so I'm more willing to
just try stuff ;)
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH 1/8] lcitool: Add "-r REVISION" argument for build
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
> > ---
> > 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 as well as
and it worked as expected, sweet. I also tried referencing
with @{}, 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@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH 1/8] lcitool: Add "-r REVISION" argument for build
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
> ---
> 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
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")
+
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH 1/8] lcitool: Add "-r REVISION" argument for build
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
---
guests/lcitool | 36 ++--
1 file changed, 26 insertions(+), 10 deletions(-)
diff --git a/guests/lcitool b/guests/lcitool
index 2901a92..c12143d 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -351,8 +351,13 @@ class Application:
metavar="PROJECTS",
help="list of projects to consider",
)
+self._parser.add_argument(
+"-r",
+metavar="REVISION",
+help="git revision to build (remote/branch)",
+)
-def _execute_playbook(self, playbook, hosts, projects):
+def _execute_playbook(self, playbook, hosts, projects, revision):
base = Util.get_base()
flavor = self._config.get_flavor()
@@ -362,6 +367,14 @@ 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:])
+
ansible_cfg_path = os.path.join(base, "ansible.cfg")
playbook_base = os.path.join(base, "playbooks", playbook)
playbook_path = os.path.join(playbook_base, "main.yml")
@@ -372,6 +385,8 @@ class Application:
"root_password_file": root_pass_file,
"flavor": flavor,
"selected_projects": selected_projects,
+"git_remote": git_remote,
+"git_branch": git_branch,
})
cmd = [
@@ -396,15 +411,15 @@ class Application:
except Exception:
raise Error("Failed to run {} on '{}'".format(playbook, hosts))
-def _action_hosts(self, _hosts, _projects):
+def _action_hosts(self, _hosts, _projects, _revision):
for host in self._inventory.expand_pattern("all"):
print(host)
-def _action_projects(self, _hosts, _projects):
+def _action_projects(self, _hosts, _projects, _revision):
for project in self._projects.expand_pattern("all"):
print(project)
-def _action_install(self, hosts, _projects):
+def _action_install(self, hosts, _projects, _revision):
base = Util.get_base()
flavor = self._config.get_flavor()
@@ -475,13 +490,13 @@ class Application:
except Exception:
raise Error("Failed to install '{}'".format(host))
-def _action_update(self, hosts, projects):
-self._execute_playbook("update", hosts, projects)
+def _action_update(self, hosts, projects, revision):
+self._execute_playbook("update", hosts, projects, revision)
-def _action_build(self, hosts, projects):
-self._execute_playbook("build", hosts, projects)
+def _action_build(self, hosts, projects, revision):
+self._execute_playbook("build", hosts, projects, revision)
-def _action_dockerfile(self, hosts, projects):
+def _action_dockerfile(self, hosts, projects, _revision):
mappings = self._projects.get_mappings()
hosts = self._inventory.expand_pattern(hosts)
@@ -553,11 +568,12 @@ class Application:
action = cmdline.a
hosts = cmdline.h
projects = cmdline.p
+revision = cmdline.r
method = "_action_{}".format(action.replace("-", "_"))
if hasattr(self, method):
-getattr(self, method).__call__(hosts, projects)
+getattr(self, method).__call__(hosts, projects, revision)
else:
raise Error("Invalid action '{}'".format(action))
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
