Here is my summary of the threads: Overwhelming agreement:
- rename `release` to something more appropriate. - add `checkstyle` to the default build (it's basically a compile error) - add more information to contributor guide Reasonable agreement - don't update the github instructions to make passing `mvn verify -P<all checks>` mandatory. Maybe add a hint that this is a good proxy for what Jenkins will run. Unresolved: - whether all checks should be in `mvn verify` - whether `mvn test` is useful for most workflows I'll propose to proceed with the overwhelmingly agreed-upon changes, and as we see increasingly many new contributors re-evaluate the remaining issues. Thanks, Dan On Tue, Jan 24, 2017 at 12:51 PM, Jean-Baptiste Onofré <j...@nanthrax.net> wrote: > +1 to at least update the contribution guide and improve the profile name. > > Regards > JB > > > On 01/24/2017 09:49 PM, Kenneth Knowles wrote: > >> My impression is that we don't have consensus on whether all checks or >> minimal checks should be the default, or whether we can have both via `mvn >> test` and `mvn verify`. >> >> But that doesn't prevent us from giving -P release a better name and >> mentioning it in the dev guide and in some manner in our PR template. >> >> Right now we are living with the combination of the bad aspects - default >> is not thorough but not actually fast and a thorough check is >> undocumented. >> >> On Tue, Jan 24, 2017 at 2:22 AM, Ismaël Mejía <ieme...@gmail.com> wrote: >> >> I just wanted to know if we have achieved some consensus about this, I >>> just >>> saw this PR that reminded me about this discussion. >>> >>> https://github.com/apache/beam/pull/1829 >>> >>> It is important that we mention the existing profiles (and the intended >>> checks) in the contribution guide (e.g. -Prelease (or -Pall-checks >>> triggers >>> these validations). >>> >>> I can add this to the guide if you like once we define the checks per >>> stage/profile. >>> >>> Ismaël >>> >>> >>> On Wed, Jan 11, 2017 at 8:12 AM, Aviem Zur <aviem...@gmail.com> wrote: >>> >>> I agree with Dan and Lukasz. >>>> Developers should not be expected to know beforehand which specific >>>> profiles to run. >>>> The phase specified in the PR instructions (`verify`) should run all the >>>> relevant verifications and be the "slower" build, while a preceding >>>> lifecycle, such as `test`, should run the "faster" verifications. >>>> >>>> Aviem. >>>> >>>> On Mon, Jan 9, 2017 at 7:57 PM Robert Bradshaw >>>> >>> <rober...@google.com.invalid >>> >>>> >>>>> wrote: >>>> >>>> On Mon, Jan 9, 2017 at 3:49 AM, Aljoscha Krettek <aljos...@apache.org> >>>>> wrote: >>>>> >>>>>> I also usually prefer "mvn verify" to to the expected thing but I see >>>>>> >>>>> that >>>>> >>>>>> quick iteration times are key. >>>>>> >>>>> >>>>> I see >>>>> https://maven.apache.org/guides/introduction/ >>>>> >>>> introduction-to-the-lifecycle.html >>>> >>>>> >>>>> verify - run any checks on results of integration tests to ensure >>>>> quality criteria are met >>>>> >>>>> Of course our integration tests are long enough that we shouldn't be >>>>> putting all of them here, but I too would expect checkstyle. >>>>> >>>>> Perhaps we could introduce a verify-fast or somesuch for fast (but >>>>> lower coverage) turnaround time. I would expect "mvn verify test" to >>>>> pass before submitting a PR, and would want to run that before asking >>>>> others to look at it. I think this should be our criteria (i.e. what >>>>> will a new but maven-savvy user run before pushing their code). >>>>> >>>>> As long as the pre-commit hooks still check everything I'm ok with >>>>>> >>>>> making >>>> >>>>> the default a little more lightweight. >>>>>> >>>>> >>>>> The fact that our pre-commit hooks take a long time to run does change >>>>> things. Nothing more annoying than seeing that your PR failed 3 hours >>>>> later because you had some trailing whitespace... >>>>> >>>>> On Thu, 5 Jan 2017 at 21:49 Lukasz Cwik <lc...@google.com.invalid> >>>>>> >>>>> wrote: >>>>> >>>>>> >>>>>> I was hoping that the default mvn verify would be the slow build >>>>>>> >>>>>> and a >>> >>>> profile could be enabled that would skip checks to make things >>>>>>> >>>>>> faster >>> >>>> for >>>>> >>>>>> regular contributors. This way a person doesn't need to have >>>>>>> >>>>>> detailed >>> >>>> knowledge of all our profiles and what they do (typically mvn >>>>>>> >>>>>> verify) >>> >>>> will >>>>> >>>>>> do the right thing most of the time. >>>>>>> >>>>>>> On Thu, Jan 5, 2017 at 9:30 AM, Dan Halperin >>>>>>> >>>>>> <dhalp...@google.com.invalid> >>>>> >>>>>> wrote: >>>>>>> >>>>>>> On Thu, Jan 5, 2017 at 9:28 AM, Jesse Anderson < >>>>>>>> >>>>>>> je...@smokinghand.com >>>> >>>>> >>>>>> wrote: >>>>>>>> >>>>>>>> @dan are you saying that mvn verify isn't doing checkstyle >>>>>>>>> >>>>>>>> anymore? >>>> >>>>> >>>>>>>> >>>>>>>> `mvn verify` alone should not be running checkstyle, if modules >>>>>>>> >>>>>>> are >>> >>>> configured correctly. >>>>>>>> >>>>>>>> >>>>>>>> Some of >>>>>>>>> the checkstyles are still running for a few modules. Also, the >>>>>>>>> >>>>>>>> contribution >>>>>>>> >>>>>>>>> docs will need to change. >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Yes. The PR includes discussion of these other needed changes, >>>>>>>> unfortunately one PR can't change two repositories. >>>>>>>> >>>>>>>> Please continue the discussion on the PR, then I will summarize it >>>>>>>> >>>>>>> back >>>>> >>>>>> into the dev thread. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Dan >>>>>>>> >>>>>>>> >>>>>>>> They say to run mvn verify before commits. >>>>>>>>> >>>>>>>>> On Thu, Jan 5, 2017 at 9:25 AM Dan Halperin >>>>>>>>> >>>>>>>> <dhalp...@google.com.invalid >>>>>>> >>>>>>>> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>> Several folks seem to have been confused after BEAM-246, where >>>>>>>>>> >>>>>>>>> we >>>> >>>>> moved >>>>>>> >>>>>>>> the >>>>>>>>> >>>>>>>>>> "slow things" into the release profile. I've started a >>>>>>>>>> >>>>>>>>> discussion >>>> >>>>> with >>>>>>> >>>>>>>> https://github.com/apache/beam/pull/1740 to see if there are >>>>>>>>>> >>>>>>>>> things >>>>> >>>>>> we >>>>>>> >>>>>>>> can >>>>>>>>> >>>>>>>>>> do to fill these gaps. >>>>>>>>>> >>>>>>>>>> Would love folks to chime in with opinions. >>>>>>>>>> >>>>>>>>>> Dan >>>>>>>>>> >>>>>>>>>> On Wed, Jan 4, 2017 at 1:34 PM, Jesse Anderson < >>>>>>>>>> >>>>>>>>> je...@smokinghand.com> >>>>>>> >>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> @Eugene, yes that failed on the checkstyle. >>>>>>>>>>> >>>>>>>>>>> On Wed, Jan 4, 2017 at 1:27 PM Eugene Kirpichov >>>>>>>>>>> <kirpic...@google.com.invalid> wrote: >>>>>>>>>>> >>>>>>>>>>> Try just -Prelease. >>>>>>>>>>>> On Wed, Jan 4, 2017 at 1:21 PM Jesse Anderson < >>>>>>>>>>>> >>>>>>>>>>> je...@smokinghand.com >>>>>>>> >>>>>>>>> >>>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>> Fails because I don't have a secret key. >>>>>>>>>>>>> >>>>>>>>>>>>> On Wed, Jan 4, 2017 at 1:03 PM Jean-Baptiste Onofré < >>>>>>>>>>>>> >>>>>>>>>>>> j...@nanthrax.net >>>>>>>>> >>>>>>>>>> >>>>>>>>>>> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> Hi Jesse, >>>>>>>>>>>>>> >>>>>>>>>>>>>> Could you try the same with: >>>>>>>>>>>>>> >>>>>>>>>>>>>> mvn verify -Prelease,apache-release >>>>>>>>>>>>>> >>>>>>>>>>>>>> ? >>>>>>>>>>>>>> >>>>>>>>>>>>>> Regards >>>>>>>>>>>>>> JB >>>>>>>>>>>>>> >>>>>>>>>>>>>> On 01/04/2017 09:53 PM, Jesse Anderson wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>>> For some reason, running "mvn verify" isn't running >>>>>>>>>>>>>>> >>>>>>>>>>>>>> checkstyle >>>>>>>> >>>>>>>>> on >>>>>>>>> >>>>>>>>>> everything. I had checkstyle errors in >>>>>>>>>>>>>>> >>>>>>>>>>>>>> beam-sdks-java-core >>>>> >>>>>> that >>>>>>>> >>>>>>>>> weren't >>>>>>>>>>>> >>>>>>>>>>>>> being found. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> I thought this was due to the extra parameters. I >>>>>>>>>>>>>>> >>>>>>>>>>>>>> reran >>>> >>>>> with >>>>>>> >>>>>>>> the >>>>>>>>> >>>>>>>>>> plain >>>>>>>>>>>> >>>>>>>>>>>>> "mvn >>>>>>>>>>>>>> >>>>>>>>>>>>>>> verify" and it still didn't find them. From the >>>>>>>>>>>>>>> >>>>>>>>>>>>>> output, >>>> >>>>> it >>>>> >>>>>> doesn't >>>>>>>>>> >>>>>>>>>>> look >>>>>>>>>>>> >>>>>>>>>>>>> like they're being run at all. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Thanks, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Jesse >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>> -- >>>>>>>>>>>>>> Jean-Baptiste Onofré >>>>>>>>>>>>>> jbono...@apache.org >>>>>>>>>>>>>> http://blog.nanthrax.net >>>>>>>>>>>>>> Talend - http://www.talend.com >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>> >>>> >>> >> > -- > Jean-Baptiste Onofré > jbono...@apache.org > http://blog.nanthrax.net > Talend - http://www.talend.com >
