[BEAM-246] Improve developer experience after streamlined Maven profile

[dhalperi]

• Rename the release profile that contains enhanced validation to all-checks
• Add the proper test command mvn clean install -Pall-checks in the GitHub Pull Request template.

[dhalperi]

FYI @kennknowles @davorbonaci @eljefe6a @lukecwik @jbonofre @aljoscha @amitsela a set of committers most recently asking about this improvement.

Comments requested.

Expect a pull request to the contribution guide as well, once discussion is resolved.

[dhalperi]

Thoughts: There is plausibly a good reason for the old release profile name, but there are really two things happening:

1. Actually building things like source jars and javadoc for the release. Hence the name release.
2. Disabling the default run of tasks and validation that are not on the critical developer path (hence putting checkstyle, etc. in a profile). Hence the name all-checks. However it's not just slow checks, it's also slow tasks -- like building source jars, shading, javadoc.

The problem is we really want to run the same set of tasks both places. Specifically, precommit should check that the release process isn't broken. And the release process should verify that all our validations are still met.

So I'm not certain which name is better, or whether we should have two profiles. But all-checks seems better for the day to day.

[bjchambers]

I guess my concern is that if we all want to run with all-checks normally, maybe that should be the default? Instead of having to turn on a profile, could we instead a profile that was like disable-slow-checks or tests-only?

This would allow our existing instructions/workflows (mvn verify) to work and is also going to be more familiar to new developers on the project that have previous Maven experience.

[asfbot]

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6428/
--none--

[eljefe6a]

I agree with @bjchambers. The issue is that checkstyle is on the developer path. I hit this yesterday with code not passing the checkstyle.

[dhalperi]

@bjchambers Running all slow checks all the time is the wrong default for Beam users and most Beam developers.

Users want to know it installs cleanly and tests pass, they don't care about building Javadoc, source jars, shaded jars, or checkstyle/rat. So mvn install should just build the installed artifacts and test them.

Most developers have some order of properties they want to ensure:

• code compiles
• tests pass
• simple validations pass like checkstyle.
• files are properly licensed.
• the source jars build.
• the
  ...
  etc.

I don't want to go back to a world where testing a simple change takes an extra 3 minutes because we're bundling and shading and Apache RAT checking all the other modules in a project.

Which was @kennknowles' entire reason for filing BEAM-246.

[kennknowles]

+1 to this PR. I've wanted to change the name -P release to something better for a while, but didn't want to deal with rocking the boat. You are right that the main things it is intended to skip are the incredibly slow jar and javadoc processes. It isn't really about checks at all, though it seems to have become so, despite its name...

I agree with @dhalperi that running all tasks/checks all the time is wrong for regular devs. I now also think it is wrong for new/rare contributors, which is a change of mind for me.

• regular devs know how things work and can readily use various invocations as long as they aren't multi-line monstrosities
• the precommit should basically match the release (modulo postcommit slow stuff)
• new contributors by definition don't know how things work, so I want to open a line of communication with them as early as possible, before all checks

I think that Maven actually has a distinction that we are not taking advantage of - mvn test versus mvn verify. The latter include packaging while the former does not. I've forgotten - is there a reason we don't use this?

[kennknowles]

I actually don't think we should add this. I want the user to immediately open a PR when it compiles. Running mvn test is a bonus. Then Jenkins & Travis can do the thorough stuff. If there is a need to iterate, the reviewer should advise on the invocation with -P all-checks.

[jasonkuster]

I think there's value in having PR authors do the all-checks stuff, since things passing or failing on your own machine is easier to debug than in Jenkins. I think the value Jenkins is adding is the ITs, plus a safety net in case people haven't done their due diligence. I don't feel super strong about this though, so I'm open to pushback.

[jasonkuster]

On the other hand, to argue against myself here a bit, if it fails in Jenkins it's way easier in the case of a new contributor for them to get help from their reviewer. No pastebinning or anything of console output from their computer, publicly-accessible by default.

[kennknowles]

My position is that "due diligence" does not apply to new contributors, to minimize the distance between them feeling like hacking and them becoming our friends :-)

And established contributors should know what to do anyhow. Or, realistically, I regularly use Jenkins to do this anyhow, since I rarely have fewer than 5 parallel threads of code and review going. We don't need a checkbox that duplicates the red/green from CI.

Letting people know the command to do a full run is fine, but making it a checkbox I'm not so happy with.

[jasonkuster]

I'm fine with it being an if-you-want instead of a checkbox. Top of my head wording below, feel free to ignore.

"For an extra-thorough check, instead of running mvn clean test try running mvn clean install -Pall-checks. This will run the code style checker and a few other checks. Don't worry too much -- Jenkins runs these too so you'll know if they're broken."

[kennknowles]

The inclusion/exclusion of jar and javadoc doesn't fit perfectly with this name. But its better the release, given that it controls a wide swath of tasks. What about just -P full?

[jasonkuster]

It feels to me almost like there should be two separate profiles - all-checks and all-(tasks? builds?). all-tasks could be a superset of all-checks, or they could be disjoint.

[jbonofre]

+1 for this change. My experience is the following:

• I'm using regular mvn clean install when working on a module/feature, and just want quick test.
• I'm using mvn clean install -Prelease,apache-release just before creating the pull request to validate. So renaming to all-checks makes sense.

[amitsela]

+1 on changing profile name.
I also agree with Kenn on the checkbox - it should be well documented in the contribution guide (how to run all test...) and probably also noted in the PR template, but it shouldn't be a perquisite to review a PR.
Worst case, a committer reviewing a (valid) PR could do small "patch-up" work on merge.