Add a vet target in order to catch suspicious constructs
reported by go vet.
Vet has successfully detected problems in the past,
for example, see
482460c4c8fc36b1cd9468a41035a97b704fb77d4f3f85b16595fa353ee94bfe18b40d
Some vet flags are noisy. In particular, the following flags
reports a large amount of generally unharmful constructs:
-assign: check for useless assignments
-composites: check that composite literals used field-keyed
elements
-shadow: check for shadowed variables
-shadowstrict: whether to be strict about shadowing
-unreachable: check for unreachable code
In order to skip running the flags mentioned above, vet is
invoked on a directory basis with 'go tool vet .' since package-
level type-checking with 'go vet' doesn't accept flags.
Hence, each file is vetted in isolation, which is weaker than
package-level type-checking. But nevertheless, it might catch
suspicious constructs that pose a real problem.
The vet target runs the following flags on the entire repo:
-asmdecl: check assembly against Go declarations
-atomic: check for common mistaken usages of the
sync/atomic package
-bool: check for mistakes involving boolean operators
-buildtags: check that +build tags are valid
-copylocks: check that locks are not passed by value
-methods: check that canonically named methods are canonically
defined
-nilfunc: check for comparisons between functions and nil
-printf: check printf-like invocations
-rangeloops: check that range loop variables are used correctly
-shift: check for useless shifts
-structtags: check that struct field tags have canonical format
and apply to exported fields as needed
-unsafeptr: check for misuse of unsafe.Pointer
Now and then, it might make sense to check the output of
VETARGS=-unreachable make vet
manually, just in case it detects several lines of dead code etc.
This was actually quite nasty as the first bug covered the second one…
The first bug is with HasChange. This function uses reflect.DeepEqual
to check if two instances are the same/have the same content. This
works fine for all types except for Set’s as they contain a function.
And reflect.DeepEqual will only say the functions are equal if they are
both nil (which they aren’t in a Set). So in effect it means that
currently HasChange will always say true for Set’s, even when they are
actually being equal.
As soon as you fix this problem, you will notice the second one (which
the added test is written for). Without saying you want the exact diff,
you will end up with a merged value which will (in most cases) be the
same.
Run all unit tests and a good part of the acc tests to verify this
works as expected and all look good.
It’s now also possible to don’t give any rules, when the firewall is
configured with `managed = true`. This will in effect mean; make sure
no rules exist at all for the firewall.
Currently the `sync.Once` call is only used to init a Set in the add()
func. So when you add a value to a Set that is the result of one of the
Set operations (i.e. union, difference, intersect) the Set will be
reinitialised and the exiting values will be lost.
I don’t have a clue why this is showing up in my ACC tests just now, as
this code is in there for quite some time already. Somehow it seems to
have something to do with the refactoring of the helper/schema done
last week, as I cannot reproduce this with
47f02f80bc
Phil Kates