Make build-and-test command configurable, add heuristics

[abesto]

Tested as:

python src/main.py  --module zipkin-api --version 0.2.1 --gpg-key 50D90C2C \
    --git-hash f6e41b694f0083eb6212bf2986f589b2cf699f50 --repo dev \
    --zipname-template 'apache-{module}{dash_incubating}-{version}-source-release' \
    --github-reponame-template '{incubator_dash}{module}.git' \
    --build-and-test-command 'yaml-lint *.yaml'

Relevant part of the output:

> Running check: Source archive builds cleanly
>> Executing `yaml-lint *.yaml` in '/tmp/tmp2_1k85yw/unzipped/zipkin-api-0.2.1'
Checking the content of ["zipkin2-api.yaml", "zipkin-api.yaml"]
File : zipkin2-api.yaml, Syntax OK
File : zipkin-api.yaml, Syntax OK
Done.

Note that currently the heuristics will run each test-and-build command that seems to be relevant. This is completely intentional (and is up for debate, should you feel the need)

[abesto]

Hwell this is awkward, @dataclass in the commit message expands to https://github.com/DataClass. Good thing it seems to be an empty org. Still, sorry for the notification mail o.0

[codefromthecrypt]

made a line comment, but I think it is busy-making to arbitrarily add a yaml verification tool just to satisfy a non-requirement. We don't check syntax of other yaml. Let's please try to make it possible to run what's required and no more, rather than adding fake things that feign tests.

[abesto]

the word error concerns me. remember source releases do not have a requirement to have tests even..

This error word is not exposed to the user though. Nitpicking aside, do you have a recommendation on what other word to use instead of error? Maybe problem? Potential problem? Also, should we replace [FAIL] with something else?

I'm fairly confident the reason for that is the scary output we have in the tooling

That's bad, what gives you that impression? Happy to tweak the output however you see fit.

instead, I'd suggest declassifying from error to notice. Ex notice: "this source release does not seem to include a build or test step."

That was my initial approach, then got worried that'd be ignored. I'll downgrade this to a notice before merging.

I think it is busy-making to arbitrarily add a yaml verification tool just to satisfy a non-requirement. We don't check syntax of other yaml. Let's please try to make it possible to run what's required and no more, rather than adding fake things that feign tests.

Oh wow. Notice how yaml-lint was passed in as an optional argument, and the error message (which I'll downgrade to a notice) even explicitly mentions you can use --build-and-test-command true to silence the error. I only used yaml-lint to showcase how this option works. I'm not proposing we add it to our usual verification process. I believe this comment is based on some misunderstanding.

[codefromthecrypt]

@abesto I don't quite understand how to phrase differently that projects that build no binary should have no requirement to run a build or test. Maybe it isn't good to mention this on the issue, but if we don't add a fake test to the command it looks like this right? What I'm trying to do is get to a point of less distractions and somehow I think people focus way too much on needing to run some test vs whether the project itself is even building something in the first place.

Ex fail is strong wording.. it is similar to -1. -1 on release votes every time has cost me personally hours, and grief, not to mention everyone else. That's why I really want us to be careful about this.

The suggestion is to hack.. and I think it is a hack to say test command = true or lint. If a project has no binaries to build, it shouldn't need to pretend it is testing that's all.

[codefromthecrypt]

PS the concrete suggestion I gave earlier was this. It might have gotten lost

notice: "this source release does not seem to include a build or test step." Then, whenever we have a .apacheverification file, we can explicitly squash that warning so that it doesn't scare people off from helping vote.

[abesto]

projects that build no binary should have no requirement to run a build or test

Got it. This has a different message to me than your original comment, which seemed to be about yaml-lint. Still downgrading this to a notice :D

Ex fail is strong wording.. it is similar to -1.

I think that fits the purpose: this tool should say [FAIL] exactly when it thinks the release vote should be a -1. #18 tracks one issue where this is not the case. I'm convinced making the lack of a build-and-test command a notice instead of a failure is another such case. Does that sound reasonable to you?

-1 on release votes every time has cost me personally hours, and grief, not to mention everyone else.

Yeah I sympathize with that, I remember how especially some of the early votes were just crazy. At the same time, as we become more familiar with ASF processes, I think project members should be encouraged to point out problems early, and our tooling should support that (when they're actual problems, as opposed to make-believe non-requirements). I feel we're aligned on that?

[abesto]

PS the concrete suggestion I gave earlier was this. It might have gotten lost

Nope, totally got it, taking care of downgrading to a notice with that wording right now.

[abesto]

How about this?

[codefromthecrypt]

:shipit: I think we have another issue about the excludes thing so this is independently an improvement

[codefromthecrypt]

Ex fail is strong wording.. it is similar to -1.

I think that fits the purpose: this tool should say [FAIL] exactly when it thinks the release vote should be a -1. #18 https://github.com/openzipkin-contrib/apache-release-verification/pull/18 tracks one issue where this is not the case. I'm convinced making the lack of a build-and-test command a notice instead of a failure is another such case. Does that sound reasonable to you?

Agree that thinking should be FAIL == -1 in ideal case. However, until there is more experience FAIL probably means "double check this!"

I think about error-prone which has the capacity to fail the compiler. Too early for this, but they bake in a notion that checks themselves need to steep a bit. https://github.com/google/error-prone/wiki/Criteria-for-new-checks

Probably what's reasonable is to only fail on strict things that are required by apache, like NOTICE. then after some experience time, promote things that are not defined but helpful to strict status, or possibly allow people to opt-into a strict fail mode (I would use this).

A future version of this be able to run against a local checkout to ensure that someone doesn't accidentally waste time by missing one small detail. I think that's a separate issue, but I find myself building the dist locally nervously scanning the files so that I don't fail later.

-1 on release votes every time has cost me personally hours, and grief, not to mention everyone else.

Yeah I sympathize with that, I remember how especially some of the early votes were just crazy. At the same time, as we become more familiar with ASF processes, I think project members should be encouraged to point out problems early, and our tooling should support that (when they're actual problems, as opposed to make-believe non-requirements). I feel we're aligned on that?

I think there are some differences about what we want to add to checks and what's required. When incubating there's a heck of a lot of problems by not progressing and certain things are simply not required or defined by ASF. We should not focus on these and rather focus on getting out of incubation. Adding "optional" checks for things not actually required are helpful

[abesto]

Coolness. Extracted two issues, I think that should capture the conclusions here. Thank you!

[codefromthecrypt]

thank you @abesto !