On 8.12.2015 12:19, David Kupka wrote:
> On 08/12/15 08:56, Petr Spacek wrote:
>> On 7.12.2015 14:41, David Kupka wrote:
>>> +def is_host_resolvable(fqdn):
>>> +    if not isinstance(fqdn, DNSName):
>>> +        fqdn = DNSName(fqdn)
>>> +    for rdtype in (rdatatype.A, rdatatype.AAAA):
>>> +        try:
>>> +            resolver.query(fqdn.make_absolute(), rdtype)
>>> +        except DNSException:
>>> +            continue
>>> +        else:
>>> +            return True
>>> +
>>> +    return False
>>>
>>
>> NACK, you are re-introducing duplicate function which was removed in
>> 498471e4aed1367b72cd74d15811d0584a6ee268.
>>
>> Please amend the patch ASAP to use new verify_host_resolvable() function so I
>> can test it and get it into 4.3.
>>
> Updated patches attached.

Hmm, my review script screams:

Detected base branch:   origin/master
Checks will be made against following base commit: 848912a add missing
/ipaplatform/constants.py to .gitignore
Writing API to API.txt
./ipalib/plugins/dns.py:2127:9: E124 closing bracket does not match visual
indentation
./ipalib/plugins/dns.py:2711:13: E128 continuation line under-indented for
visual indent
./ipalib/plugins/dns.py:2713:9: E124 closing bracket does not match visual
indentation
./ipalib/plugins/dns.py:2716:13: E128 continuation line under-indented for
visual indent
./ipapython/ipautil.py:928:1: E302 expected 2 blank lines, found 1
./ipapython/ipautil.py:948:17: E128 continuation line under-indented for
visual indent
./ipapython/ipautil.py:956:1: E302 expected 2 blank lines, found 1
./ipapython/ipautil.py:997:80: E501 line too long (83 > 79 characters)
./ipapython/ipautil.py:998:80: E501 line too long (83 > 79 characters)
./ipaserver/install/bindinstance.py:49:9: E128 continuation line
under-indented for visual indent
./ipaserver/install/bindinstance.py:292:80: E501 line too long (80 > 79
characters)
./ipaserver/install/bindinstance.py:438:19: E128 continuation line
under-indented for visual indent
./ipaserver/install/server/common.py:271:63: E261 at least two spaces before
inline comment
./ipaserver/install/server/common.py:271:80: E501 line too long (90 > 79
characters)
ERROR: PEP8 --diff failed, continuing ...
ERROR: API.txt was changed without a change in VERSION, continuing ...
Summary of detected errors:
 ERROR: PEP8 --diff failed
 ERROR: API.txt was changed without a change in VERSION

Please fix it :-)

Make make this more pleasant for you, you can use (and review) my attached
patch. It adds 'review' target to Makefile, it will do the same checks as I do.

-- 
Petr^2 Spacek
make lint is running ... make[1]: warning: jobserver unavailable: using -j1.  Add '+' to parent make rule.
Building IPA 4.2.90.201512081417GIT8d05032
~/pkg/ipa/git ~/pkg/ipa/git/install/po
install/po/tmp.pot.update: warning: Charset "CHARSET" is not a portable encoding name.
                                    Message conversion to user's charset might not work.
~/pkg/ipa/git/install/po
tmp.pot updated
tmp.pot: 3112 entries, 3155 msgid, 0 msgstr, 0 warnings 0 errors
Detected base branch:   origin/master
Checks will be made against following base commit: 848912a add missing /ipaplatform/constants.py to .gitignore
Writing API to API.txt
./ipalib/plugins/dns.py:2127:9: E124 closing bracket does not match visual indentation
./ipalib/plugins/dns.py:2711:13: E128 continuation line under-indented for visual indent
./ipalib/plugins/dns.py:2713:9: E124 closing bracket does not match visual indentation
./ipalib/plugins/dns.py:2716:13: E128 continuation line under-indented for visual indent
./ipapython/ipautil.py:928:1: E302 expected 2 blank lines, found 1
./ipapython/ipautil.py:948:17: E128 continuation line under-indented for visual indent
./ipapython/ipautil.py:956:1: E302 expected 2 blank lines, found 1
./ipapython/ipautil.py:997:80: E501 line too long (83 > 79 characters)
./ipapython/ipautil.py:998:80: E501 line too long (83 > 79 characters)
./ipaserver/install/bindinstance.py:49:9: E128 continuation line under-indented for visual indent
./ipaserver/install/bindinstance.py:292:80: E501 line too long (80 > 79 characters)
./ipaserver/install/bindinstance.py:438:19: E128 continuation line under-indented for visual indent
./ipaserver/install/server/common.py:271:63: E261 at least two spaces before inline comment
./ipaserver/install/server/common.py:271:80: E501 line too long (90 > 79 characters)
ERROR: PEP8 --diff failed, continuing ...
ERROR: API.txt was changed without a change in VERSION, continuing ...
Summary of detected errors:
 ERROR: PEP8 --diff failed
 ERROR: API.txt was changed without a change in VERSION

Please see /tmp/tmp.WGHe4ApoK3.log
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=""
+CURRBRANCH="$(git branch --remote --contains)"
+while [ "${BASEBRANCH}" == "" ]
+do
+	BASEBRANCH="$(git branch --remote --contains "HEAD~${PATCHCNT}" | grep -v "^. ${CURRBRANCH}$" || :)"
+	PATCHCNT="$(expr "${PATCHCNT}" + 1)"
+done
+echo "Detected base branch: ${BASEBRANCH}"
+echo -n "Checks will be made against following base commit: "
+git log -1 --oneline ${BASEBRANCH}
+
+# gather all errors in one array and print error summary at the end
+declare -a errs
+errs=("Summary of detected errors:\n")
+
+is_tree_clean || log_error "Git tree must be clean before you start review"
+
+./makeapi
+is_tree_clean "API.txt" || log_error "./makeapi changed something"
+
+./makeaci
+is_tree_clean "ACI.txt" || log_error "./makeaci changed something"
+
+git diff ${BASEBRANCH} -U0 | pep8 --diff || log_error "PEP8 --diff failed"
+
+# if API.txt is changed require change in VERSION
+if ! git diff ${BASEBRANCH} --quiet -- API.txt;
+then
+	git diff ${BASEBRANCH} --quiet -- VERSION && log_error "API.txt was changed without a change in VERSION"
+fi
+
+# print error summary
+if [ "${#errs[*]}" != "1" ]
+then
+	echo -e "${errs[*]}"
+	echo "Please see ${LOGFILE}"
+	exit 1
+else
+	rm "${LOGFILE}"
+	echo "review.sh did not detect any problem"
+fi
-- 
2.5.0

-- 
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

Reply via email to