[libvirt-ci PATCH v2 00/13] Introduce a new global YAML config file for lcitool

Erik Skultety posted 13 patches 2 weeks ago
Failed in applying to current master (apply log)
guests/README.markdown                    |  18 +-
guests/config.yaml                        |  42 +++++
guests/group_vars/all/install.yml         |  11 --
guests/{lcitool => lcitool.py}            | 209 +++++++++++-----------
guests/playbooks/build/main.yml           |   2 +-
guests/playbooks/update/main.yml          |   6 +-
guests/playbooks/update/tasks/gitlab.yml  |   4 +-
guests/playbooks/update/tasks/kludges.yml |   2 +-
guests/playbooks/update/tasks/users.yml   |  42 ++---
guests/requirements.txt                   |   4 +
guests/test_config.py                     | 165 +++++++++++++++++
11 files changed, 353 insertions(+), 152 deletions(-)
create mode 100644 guests/config.yaml
delete mode 100644 guests/group_vars/all/install.yml
rename guests/{lcitool => lcitool.py} (88%)
create mode 100644 guests/requirements.txt
create mode 100644 guests/test_config.py

[libvirt-ci PATCH v2 00/13] Introduce a new global YAML config file for lcitool

Posted by Erik Skultety 2 weeks ago
This series is trying to consolidate the number of config files we currently
recognize under ~/.config/lcitool to a single global YAML config file. Thanks
to this effort we can expose more seetings than we previously could which will
come handy in terms of generating cloudinit images for OpenStack.

Patches 1-4 (ACKed)

Since RFC:
- replaced TOML with YAML which simplified some aspects of the code, thanks
Andrea
- instead of hardcoding the default values, the config within the repo is used
as a template and overriden with user-selected options

Since v1:
- uncommented the mandatory options in the default template YAML config so that
  we know about all the supported keys which is useful for validating the user
  config
- removed guests/group_vars/all/install.yaml in patch 11 (which I forgot in v1)
- added checks for value types we get from the config
- use yaml.safe_load instead of yaml.load
- added code snippet to delete keys we don't recognize so as not to introduce a
  security issue, because we essentially just take the config and pass it to
  ansible, we don't want users to use to re-define some of Ansible's variables
- added the last patch just to demonstrate a number of test cases I used as a
  proof for correctness of this revision (feel free to add more cases), but
  this is not the right series to add pytest support into lcitool

Erik Skultety (13):
  requirements: Introduce a requirements.txt file
  lcitool: Decrease the indent when creating a tempdir for initrd
    injection
  lcitool: Prefer tempfile's native wrappers over low level primitives
  lcitool: Use a temporary JSON file to pass extra variables
  config: Introduce a new global config.yaml configuration file
  lcitool: Introduce methods to load and validate the YAML config
  lcitool: Update the config values with internal playbook settings
  lcitool: Drop the get_flavor() method
  lcitool: Drop the get_root_password_file() method
  lcitool: Drop the gitlab-related getter methods
  config: Move the virt-install settings from install.yml to the config
  guests: README: Document the existence and usage of config.yaml
  DO NOT MERGE: Demonstrate functionality with pytest unit tests

 guests/README.markdown                    |  18 +-
 guests/config.yaml                        |  42 +++++
 guests/group_vars/all/install.yml         |  11 --
 guests/{lcitool => lcitool.py}            | 209 +++++++++++-----------
 guests/playbooks/build/main.yml           |   2 +-
 guests/playbooks/update/main.yml          |   6 +-
 guests/playbooks/update/tasks/gitlab.yml  |   4 +-
 guests/playbooks/update/tasks/kludges.yml |   2 +-
 guests/playbooks/update/tasks/users.yml   |  42 ++---
 guests/requirements.txt                   |   4 +
 guests/test_config.py                     | 165 +++++++++++++++++
 11 files changed, 353 insertions(+), 152 deletions(-)
 create mode 100644 guests/config.yaml
 delete mode 100644 guests/group_vars/all/install.yml
 rename guests/{lcitool => lcitool.py} (88%)
 create mode 100644 guests/requirements.txt
 create mode 100644 guests/test_config.py

-- 
2.25.3


Re: [libvirt-ci PATCH v2 00/13] Introduce a new global YAML config file for lcitool

Posted by Andrea Bolognani 2 weeks ago
On Tue, 2020-05-12 at 16:42 +0200, Erik Skultety wrote:
> This series is trying to consolidate the number of config files we currently
> recognize under ~/.config/lcitool to a single global YAML config file. Thanks
> to this effort we can expose more seetings than we previously could which will
> come handy in terms of generating cloudinit images for OpenStack.
> 
> Patches 1-4 (ACKed)
> 
> Since RFC:
> - replaced TOML with YAML which simplified some aspects of the code, thanks
> Andrea
> - instead of hardcoding the default values, the config within the repo is used
> as a template and overriden with user-selected options
> 
> Since v1:
> - uncommented the mandatory options in the default template YAML config so that
>   we know about all the supported keys which is useful for validating the user
>   config
> - removed guests/group_vars/all/install.yaml in patch 11 (which I forgot in v1)
> - added checks for value types we get from the config
> - use yaml.safe_load instead of yaml.load
> - added code snippet to delete keys we don't recognize so as not to introduce a
>   security issue, because we essentially just take the config and pass it to
>   ansible, we don't want users to use to re-define some of Ansible's variables
> - added the last patch just to demonstrate a number of test cases I used as a
>   proof for correctness of this revision (feel free to add more cases), but
>   this is not the right series to add pytest support into lcitool

You should have all the ACKs you need now.

Thanks for being patient during review, and for taking care of this
in the first place :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt-ci PATCH v2 00/13] Introduce a new global YAML config file for lcitool

Posted by Erik Skultety 2 weeks ago
On Wed, May 13, 2020 at 07:00:49PM +0200, Andrea Bolognani wrote:
> On Tue, 2020-05-12 at 16:42 +0200, Erik Skultety wrote:
> > This series is trying to consolidate the number of config files we currently
> > recognize under ~/.config/lcitool to a single global YAML config file. Thanks
> > to this effort we can expose more seetings than we previously could which will
> > come handy in terms of generating cloudinit images for OpenStack.
> > 
> > Patches 1-4 (ACKed)
> > 
> > Since RFC:
> > - replaced TOML with YAML which simplified some aspects of the code, thanks
> > Andrea
> > - instead of hardcoding the default values, the config within the repo is used
> > as a template and overriden with user-selected options
> > 
> > Since v1:
> > - uncommented the mandatory options in the default template YAML config so that
> >   we know about all the supported keys which is useful for validating the user
> >   config
> > - removed guests/group_vars/all/install.yaml in patch 11 (which I forgot in v1)
> > - added checks for value types we get from the config
> > - use yaml.safe_load instead of yaml.load
> > - added code snippet to delete keys we don't recognize so as not to introduce a
> >   security issue, because we essentially just take the config and pass it to
> >   ansible, we don't want users to use to re-define some of Ansible's variables
> > - added the last patch just to demonstrate a number of test cases I used as a
> >   proof for correctness of this revision (feel free to add more cases), but
> >   this is not the right series to add pytest support into lcitool
> 
> You should have all the ACKs you need now.
> 
> Thanks for being patient during review, and for taking care of this
> in the first place :)

Thanks for the review, changes are now merged.

-- 
Erik Skultety