[libvirt-jenkins-ci PATCH v3 4/5] guests: Introduce the new 'gitlab' flavor

[Erik Skultety]

On Tue, Apr 14, 2020 at 12:31:26PM +0200, Andrea Bolognani wrote:
> On Tue, 2020-04-14 at 10:17 +0200, Erik Skultety wrote:
> > On Thu, Apr 09, 2020 at 12:28:50PM +0200, Andrea Bolognani wrote:
> > > On Thu, 2020-04-09 at 06:23 +0200, Erik Skultety wrote:
> > > > +++ b/guests/playbooks/update/tasks/gitlab.yml
> > > > +- name: Make {{ gitlab_runner_config_dir }} world readable
> > > > +  file:
> > > > +    path: '{{ gitlab_runner_config_dir }}'
> > > > +    mode: '0755'
> > > > +
> > > > +- name: Make {{ gitlab_runner_config_dir }}/config.toml world readable
> > > > +  file:
> > > > +    path: '{{ gitlab_runner_config_dir }}/config.toml'
> > > > +    mode: '0644'
> > >
> > > The message for these tasks is unnecessarily detailed: I'd just use
> > > something like
> > >
> > >   Make gitlab-runner configuration readable
> >
> > Okay, however...
> >
> > > for both.
> > >
> > > Additionally, even though the gitlab user is going to be the only one
> > > on the system so it doesn't make much of a difference in practice, I
> > > think we should have config.toml
> > >
> >
> > ...here you suggest the following adjustment. I feel like the messages above
> > will then become confusing and misleading, since who are we making it readable
> > for? Well, only for the gitlab user, so I think a little more detail in them is
> > justifiable.
> >
> > >   owner: root
> > >   group: gitlab
> > >   mode: '0640'
> >
> > So how about:
> > "Make gitlab-runner config dir readable" for the former and
> > "Make gitlab-runner config.toml owned by the gitlab group" for the latter
>
> I still think that's an unnecessary amount of detail, and we have
> plenty of existing examples in the repository such as
>
>   - name: Update installed packages
>     package:
>       name: fedora-gpg-keys
>       state: latest
>       disable_gpg_check: yes
>     when:
>       - os_name == 'Fedora'
>       - os_version == 'Rawhide'
>
>   - name: Update installed packages
>     command: '{{ package_manager }} update --refresh --exclude "kernel*" -y'
>     args:
>       warn: no
>     when:
>       - os_name == 'Fedora'
>       - os_version == 'Rawhide'
>
>   - name: Update installed packages
>     command: '{{ package_manager }} update --disablerepo="*" --enablerepo=fedora-rawhide-kernel-nodebug "kernel*" -y'
>     args:
>       warn: no
>     when:
>       - os_name == 'Fedora'
>       - os_version == 'Rawhide'

Fair enough, ^this is a compelling counter-argument, I proceeded with the
original suggestion and merged the series, thanks for the review.

--
Erik Skultety