On (11/12/15 09:46), Petr Spacek wrote:
>On 11.12.2015 09:22, Lukas Slebodnik wrote:
>> On (10/12/15 18:04), Petr Spacek wrote:
>>> On 9.12.2015 15:30, Petr Spacek wrote:
>>>> Hello,
>>>>
>>>> this patch automates some of sanity checks proposed by Petr Vobornik.
>>>>
>>>> 'make review' should be used in root of clean Git tree which has patches
>>>> under
>>>> review applied.
>>>>
>> +1 for automatisation
>> -1 for name.
>>
>> Your script does not review[1] anything. Code review(peer review) is a job
>> for humans. What do you think about alternative
>> names: "review-helper", "sanity-check" ...
>
>I like short name but I respect your point, so I've updated the script to
>print:
>"No problems detected using lint, pep8, ./make{api,aci}, and VERSION" at the
>end.
>Now it should be clear what it did, so reviewer knows what steps were already
>done and what can be skipped during further manual review.
>
"review-helper", "sanity-check" are not long.
long would be
"aotomated-part-of-sanity-checks-from-FreeIPA-developers's-check-list"
And pdf document
https://pvoborni.fedorapeople.org/FreeIPAdeveloperschecklist.pdf
already mentioned "Sanity checks"
Please consider to not use confusing name.
Neither as make target nor as name of script.
BTW I cannot see sanity check for spec file.
If it will be added to FreeIPAdeveloperschecklist.pdf
than you might add it to the sript as well.
>Attached patch also adds 'assert-clean-tree' target which is run before lint
>target, so no time is wasted if the tree is not clean. Parallel build will
>execute and terminate both targets at the same time, but it saves time in both
>cases, the output is just not so nice.
>
>And IPA cannot be built in parallel anyway :-)
>
>>>> Magic in review.sh attempts to detect nearest remote branch which can be
>>>> used
>>>> as diff base for review. Please see review.sh for further details.
>>>
>>> And here is the patch! :-)
>>>
>>> --
>>> Petr^2 Spacek
>>
>> [1] https://en.wikipedia.org/wiki/Code_review
>>
>>>From 52393bf2bb0ad74dbe37e496b1fd41a6ab22bd90 Mon Sep 17 00:00:00 2001
>>> From: Petr Spacek <pspa...@redhat.com>
>>> Date: Tue, 8 Dec 2015 12:06:33 +0100
>>> Subject: [PATCH] Add 'review' target for make. It automates following tasks:
>>>
>>> - check if ACI.txt and API.txt are up-to-date
>>> - check if VERSION was changed if API was changed
>>> - pep8 --diff does not produce new errors when ran on diff from
>>> origin/branch
>>> - make lint does not produce errors
>>> ---
>>> Makefile | 4 ++++
>>> review.sh | 64
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 68 insertions(+)
>>> create mode 100755 review.sh
>>>
>>> diff --git a/Makefile b/Makefile
>>> index
>>> d2c37f1597a011af2bd9ef1a4f7ce87ac59620a3..b85b3a5d6362150dcebf16745414c3d416c9547f
>>> 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -79,6 +79,10 @@ check: bootstrap-autogen server tests
>>> (cd $$subdir && $(MAKE) check) || exit 1; \
>>> done
>>>
>>> +# works only in Git tree
>>> +review: version-update
>>> + ./review.sh
>>> +
>>> bootstrap-autogen: version-update client-autogen
>>> @echo "Building IPA $(IPA_VERSION)"
>>> cd asn1; if [ ! -e Makefile ]; then ../autogen.sh --prefix=/usr
>>> --sysconfdir=/etc --localstatedir=/var --libdir=$(LIBDIR); fi
>>> diff --git a/review.sh b/review.sh
>>> new file mode 100755
>>> index
>>> 0000000000000000000000000000000000000000..3a5114fbe88d3aba8e926e049500f5ac5f41a83e
>>> --- /dev/null
>>> +++ b/review.sh
>>> @@ -0,0 +1,64 @@
>>> +#!/bin/bash
>>> +is_tree_clean() {
>>> + test "$(git status --porcelain "$@")" == ""
>>> + return $?
>>> +}
>>> +
>>> +log_error() {
>>> + echo "ERROR: ${1}, continuing ..."
>>> + errs=(${errs[@]} "ERROR: ${1}\n")
>>> +}
>>> +
>>> +set -o errexit -o nounset #-o xtrace
>>> +
>>> +LOGFILE="$(mktemp --suff=.log)"
>>> +exec > >(tee -i ${LOGFILE})
>>> +exec 2>&1
>>> +
>>> +echo -n "make lint is running ... "
>>> +make --silent lint || log_error "make lint failed"
>>> +
>>> +# Go backwards in history until you find a remote branch from which current
>>> +# branch was created. This will be used as base for git diff.
>>> +PATCHCNT=0
>>> +BASEBRANCH=""
>> If base branch means the remote branch which was used for cloning the cerrent
>> git HEAD than you can simplify it a littl bit.
>> git rev-parse --symbolic-full-name @{upstream}
>
>Nice! I did not know this trick. Unfortunately it does not work for my
>use-case: I clone upstream repo, pick a branch to base review on, create my
>own branch from the upstream one, and then apply patches on top of that.
>
>Imagine that I'm going to run the tool on branch 'review-tool'. Git history
>tree looks like this:
>
>* d036f3e (HEAD -> review-tool) fixup! Add 'review' target for make.
>* c6d75b0 Makefile: disable parallel build
>| * 667d50b (dkupka-review) dns: Add --auto-reverse option.
>| * c85f176 dns: Check if domain already exists.
>| * a55cd76 dns: do not add (forward)zone if it is already resolvable.
>| * 0b0c529 (dns-no-forwarders) DNS: Make --no-forwarders option default.
>|/
>* d2e8470 Add 'review' target for make. It automates following tasks:
>| * 101ffae (mbasti-review) Fix DNS tests: dns-resolve returns warning
>| * 52393bf Add 'review' target for make. It automates following tasks:
>|/
>* 848912a (origin/master, master) add missing /ipaplatform/constants.py to
>.gitignore
>
>$ git rev-parse --symbolic-full-name @{upstream}
>errors out like this:
>fatal: no upstream configured for branch 'review-tool'
>
>I definitely agree that the code above is just an ugly hack, but I would like
>to keep current behavior so I do not need to change my workflow.
>
good point.
But work with arrays nad complicated logic in bash is not very nice :-)
Almost impossible to find out what it does.
If you do not plan to rewrite it to normal scripting language :-)
than please use bash function for this as well.
LS
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
