+1 It sounds good to me.
Thanks Dan ! Regards JB On Jan 25, 2017, 19:39, at 19:39, Dan Halperin <dhalp...@google.com.INVALID> wrote: >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 >>
