So I think that we are more interested in running ansible-lint https://github.com/ansible/ansible-lint rather than ansible-report.

Ok we want https://github.com/willthames/ansible-review :-)

The following email was sent by cverna to the Fedora Infrastructure list on Fri 25 Oct 2019 01:04 PM UTC.

It is included here to help keep the discussion in the ticket current.

Hi all,

Today jlanda, austinpowered and mizdebsk discussed about ticket https://pagure.io/fedora-infrastructure/issue/8157 in #fedora-admin and came up with a few questions on how to implement that solution that I think would be nice to share with the wider group.

There are basically 2 possibility :

1 - We run ansible-report as a pre-commit hook
This means that ansible-report will be run locally before a contributor commit a change. This is not ideal since our contributor are running all kind systems (rhel, fedora, windows ?) so having something that work well for everyone will not be simple. Also this forces our contributors to install ansible-report locally.

2 - We run ansible-report as a pre-receive hook
This means that ansible-report is run on batcave01, but we cannot run ansible-report just on a commit, we need to run the tool against the full repository every time. That involve making a clone of the repo applying the changes in the incoming commit, then run ansible-report on that repository.

This has also a few disadvantages, first we first need to clear all the errors reported by ansible-report in our repo before we enable the hook otherwise all commits will be rejected. It will also slows down every pushes (time to clone, apply patch, run the tool).

Do people have other ideas ? Is this change worth the trouble ?

Thanks

Also mentioned earlier today in the chat by @jlanda was using standard CI workflow with PR's and auto-merging. As mentioned, this would be a completely different approach.

We could discuss the merits of this plan here before we decide on a path forward.

@jlanda - I start to feel that PRs, an standard CI system and automerging on success will give us less headaches on the long-term than a bare git hooks custom solution :)
@cverna - kinds of feel like @jlanda but having PRs for this repo is another story
@jlanda - Yeah, but this solution will be a big story too :) and we'll have to keep it running

So I played a bit on that today, and I think that in the end we should be able to use a git hook.

=== EDIT ===
Ok it seems that it is not that easy to have it run as a pre-receive hook. ansible-review is quite picky on the input it gets.

===========

I worked on the following pre-receive hook, I can test it to much since my silverblue toolbox container is broken for some reason :(

but to test it locally I did something like that

git clone --bare https://infrastructure.fedoraproject.org/infra/ansible.git/
mv ansible.git/hooks/pre-receive.sample ansible.git/hooks/pre-receive
# copy the following script in the pre-receive hook
git clone ansible.git
cd ansible
# make some changes in yml files
git commit -am "Some changes"
git push origin master
# see the hook output.

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26

#!/usr/bin/env bash

set -e

oldrev=$1
newrev=$2
refname=$3

while read oldrev newrev refname; do
    # Get a list of all objects in the new revision
    objects=`git ls-tree --full-name -r ${newrev}`

    # Get the file names, without directory, of the files that have been modified
    # between the new revision and the old revision
    git diff --name-only $oldrev $newrev | while read file; do
        # Search for the file name in the list of all objects
        object=`echo -e "${objects}" | egrep "(\s)${file}\$" | egrep '\.yml$' | awk '{ print $3 }'`
        # If it's not present, then continue to the the next itteration
        if [ -z ${object} ];
        then
            continue;
        fi
        echo $file
        git show $newrev:$file | ansible-review || exit 1
    done
done

ansible-review can be run as follow

$ sudo dnf install ansible-review
$ git clone https://infrastructure.fedoraproject.org/infra/ansible.git/
$ cd ansible
$ find . -name "*.yml" | xargs ansible-review

@cverna Maybe stupid questions but:

1. How are you going to enforce that a pre-commit hook is run client-side? AIUI, they're aimed at the contributor to automate things they want automated on their side (like spell-checking the commit-log). There's no way to know if it was executed, or even if they have one.

2. Both pre-commit and pre-receive hook scripts get the same information, i.e. old-rev new-rev rev-name per rev updated. What's the issue with running ansible-review server-side then?

@cverna Maybe stupid questions but:

There are no stupid questions :smile:

How are you going to enforce that a pre-commit hook is run client-side? AIUI, they're aimed at the contributor to automate things they want automated on their side (like spell-checking the commit-log). There's no way to know if it was executed, or even if they have one.

Yes we don't really want to enforce it client side, that's why the pre-commit hook would not really work. If we want to do that in the server side then a pre-receive hook could be configured on the repo on batcave01 and check every commit that is pushed.

Both pre-commit and pre-receive hook scripts get the same information, i.e. old-rev new-rev rev-name per rev updated. What's the issue with running ansible-review server-side then?

Yes from the little playing I did, ansible-review is quite picky on what you pass to it I did not manage to have to work by passing it only the diff from a commit.

Quick update:

• It looks as if we had a pretty old version of it (0.13.7 from June 2018), which lacks some important fixes in the current one (0.13.9, still from November last year), namely "Fix reading from stdin in python 3".
• I packaged that version and submitted updates for F-30, 31, and tested with that version. However, it apparently uses API around inventories that doesn't exist anymore because it was restructured in Ansible 2.4.
• I've patched it up to work with the new InventoryManager API, and it seems to be working. But I'm not at all familiar with Ansible code, so ran my changes past @misc and @toshio (thanks guys!) before submitting it as a PR: https://github.com/willthames/ansible-review/pull/85
• I've added the patch to the Fedora packages. Please test: F30 F31
• For testing, I'm letting it run against the last 2000 commits, yesterday I did a shorter run with 300. It seems to hold up well with one caveat: it expects the source tree to match the state of the diff, but this shouldn't matter much for the purpose of a pre-receive hook script.

NB: For arbitrary commits, having a shallow clone of the later one and using the diff between the older and the later as an input to ansible-review could work, too.

Today I've written a pre-receive hook script which runs ansible-review on every commit in a push, it held up well for master~1000..master, i.e. only rejected a push if it would introduce a problematic commit (syntax errors, rules referencing non-existent files, ...).

Here it is: https://github.com/nphilipp/ansible-review-git-hook

Currently, it will spit out three warnings per commit because it doesn't find configuration except what comes with the program, we might want to add that just to reduce the output of it. I didn't encounter one, but if there is a bug with ansible-review preventing a legit change to be pushed, simply making the hook not executable for the moment does the trick. It's what I did to skip over the problematic commits in my test set. :wink:

Merry Christmas everybody! :santa::christmas_tree::snowman:

So now that the ansible repo has migrated to pagure, @nphilipp and I have spoken about simply running ansible-review on PRs using zuul.

I've started on the configuration to get zuul to run tests on PRs and @nphilipp has agreed to look again at ansible-review with zuul.

Proposed final fix for this is in fedora-infra/ansible#54, which can't be merged because our Zuul is too old. We have workarounds for that in place, see fedora-infra/ansible#60 and fedora-zuul-jobs#60, so I'll close this ticket.