+1 On Jan 25, 2017 11:15, "Jean-Baptiste Onofré" <j...@nanthrax.net> wrote:
> +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 > >> >
