Can we consider adding rat-plugin and findbugs to the default verify phase? Currently they only run when the `release` profile is enabled.
On Thu, Jan 26, 2017 at 11:42 AM Aljoscha Krettek <aljos...@apache.org> wrote: > +1 to what Dan said > > On Wed, 25 Jan 2017 at 21:40 Kenneth Knowles <k...@google.com.invalid> > wrote: > > > +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 > > > >> > > > > > >
