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
