Vendor check to succeed even if copyright information is not found #40489
Conversation
|
Curious: was there a specific dependency where this check failed? |
|
I observed a failure relating to |
|
Looks like their "master" branch has a correct But the Best way forward would be to (in order of preference);
Note that (I think) Docker EE was still on their v2.x version, which has been deprecated (I recall working on changing to v3.0, and opening a few PR's with them, e.g. segmentio/analytics-go#150 and segmentio/analytics-go#151), but the package looks to be largely unmaintained. If Mirantis is a customer, perhaps try to get some attention to it through those channels (as it's quite bad that they don't maintain the package). |
|
Sure, but this function still seems broken, at least according to the documentation. |
|
Ah, you're right, I didn't notice that the intent was to only "warn" (and not fail); Line 39 in 545e817 |
hack/validate/vendor
Outdated
| @@ -40,7 +40,7 @@ validate_vendor_diff(){ | |||
| validate_vendor_used() { | |||
| for f in $(mawk '/^[a-zA-Z0-9]/ { print $1 }' vendor.conf); do | |||
| if [ -d "vendor/$f" ]; then | |||
| found=$(echo "vendor/$f/"* | grep -iEc '/(LICENSE|COPYING)') | |||
| found=$(echo "vendor/$f/"* | grep -iEc '/(LICENSE|COPYING)' || true) | |||
| if [ "$found" -eq 0 ]; then | |||
There was a problem hiding this comment.
If we're checking the output of grep and that's all this is for, then we should just check the return value directly instead, ala:
if ! echo ... | grep -qiEc ...; then(Sorry for brevity, on my phone but I think you get the idea)
| @@ -40,8 +40,7 @@ validate_vendor_diff(){ | |||
| validate_vendor_used() { | |||
| for f in $(mawk '/^[a-zA-Z0-9]/ { print $1 }' vendor.conf); do | |||
| if [ -d "vendor/$f" ]; then | |||
| found=$(echo "vendor/$f/"* | grep -iEc '/(LICENSE|COPYING)') | |||
| if [ "$found" -eq 0 ]; then | |||
| if ! echo "vendor/$f"/* | grep -qiEc '/(LICENSE|COPYING)'; then | |||
There was a problem hiding this comment.
@tianon Is this more agreeable with you?
|
@tianon I don't think this is quite right; the script sources I think we will need to capture and analyze the output of grep -c and ignore the return code to avoid an early exit. |
|
ping @thaJeztah @tianon PTAL I had to change this back because of the fact that pipefail is set. All return codes must be 0 or the validation phase will fail. |
|
Ah, just had this tab open and wanted to ask if you verified that it didn't work with those changes 😅 |
thaJeztah
added
area/testing
kind/bugfix
|
Neither of $ ( set -Eeuo pipefail; echo first; false; echo second ); echo $?
first
1
$ ( set -Eeuo pipefail; echo first; false | true; echo second ); echo $?
first
1
$ ( set -Eeuo pipefail; echo first; true | false; echo second ); echo $?
first
1
$ ( set -Eeuo pipefail; echo first; if false; then echo false; fi; echo second ); echo $?
first
second
0
$ ( set -Eeuo pipefail; echo first; if false | true | false; then echo false; fi; echo second ); echo $?
first
second
0 |
The documentation for validate_vendor_used in hack/validate/vendor states that a warning will be emitted if license information cannot be found in a vendored package. However, because the script is run with pipefail set (owing to the inclusion of the common validation script .validate) and `grep -c` is used, the entire script will fail whenever license information cannot be found in a vendored package. Signed-off-by: Chris Price <[email protected]>
TIL. Thanks and sorry for my confusion! |
|
Windows failure is unrelated, and being discussed in #40512 |
- What I did
The documentation for validate_vendor_used in hack/validate/vendor states
that a warning will be emitted if license information cannot be found in
a vendored package. However, because the script is run with pipefail set
(owing to the inclusion of the common validation script .validate) and
grep -cis used, the entire script will fail whenever license informationcannot be found in a vendored package.
I updated the hack/validate/vendor script to ensure the function's behaviour
is consistent with the documentation.
- Description for the changelog
Vendor check to succeed even if copyright information is not found