On Wed, Aug 22, 2018 at 01:12:58PM +0200, Andrea Bolognani wrote:
> On Wed, 2018-08-22 at 12:37 +0200, Erik Skultety wrote:
> > On Wed, Aug 22, 2018 at 11:44:17AM +0200, Andrea Bolognani wrote:
> > > Instead of defining a default and overriding it on a
> > > case-by-case basis, always define archive_format at the
> > > project level.
> > >
> > > This adds a bit of duplication, but it's consistent with
> > > how we define other metadata for projects and it will
> > > help us out later.
> >
> > How is it going to help us later?
>
> Welp, I should definitely have expanded on those "will help us out
> later" bits O:-)
>
> > So here you move something which is perfectly
> > fine to have in defaults and override it only when necessary and in the
> > following patch you're making a preparation changes for essentially the
> > opposite of this - moving autogen_args and similar to defaults (not that any
> > template needs to override these at the moment, but I'm getting confused by
> > the
> > consistency you talk about).
>
> The ultimate goal is to translate the JJB jobs definitions to
> Ansible tasks while keeping the two as close as possible so that
> further changes can be applied pretty much verbatim to both and
> not make maintainance a nightmare.
>
> Problem is, some JJB constructs are awfully difficult to translate
> without adding extra boilerplate, so in those cases I opted for
> moving to a different construct instead.
>
> So for example autogen_args can be defined as a default because
> every time we need to override it we can use
>
> - autotools-build-job:
> autogen_args: --enable-gtk-doc
>
> which translates quite naturally to
>
> - include: '{{ playbook_base }}/jobs/autotools-build-job.yml'
> vars:
> autogen_args: --enable-gtk-doc
>
> since variables passed to an included task are limited in scope
> to the included task and don't affect any other module call, but
> for archive_format we can't do the same because we want to
> translate
>
> - project:
> name: libvirt-dbus
> archive_format: xz
>
> to
>
> - set_fact:
> name: libvirt-dbus
> archive_format: xz
>
> and set_fact affects the *global* state, which means that it will
> overwrite the default every time it is called and subsequent tasks
> might break depending on the order they're called in.
Understood.
>
> I guess I could create a task that takes care of stashing the
Nah, that would just cause more confusion. I'd appreciate if you managed to
put a much condensed version of your explanation into the commit message for
future generations :).
Reviewed-by: Erik Skultety <eskul...@redhat.com>
> existing defaults before overriding them with the
> project-appropriate ones, and another one that restores the
> previous values after running all tasks for a project, but as I
> said that's extra boilerplate that we can avoid thanks to this
> patch.
>
> (As an aside, I actually like having archive_format spelled out
> explicitly along with other project metadata such as name, title
> and git_url: it makes sense to me to have that kind of information
> all in one place. But the above is the actual reason why this
> patch is needed.)
>
> --
> Andrea Bolognani / Red Hat / Virtualization
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
