+1 On Wed, Jan 25, 2017 at 10:38 AM, Dan Halperin <[email protected]> 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é <[email protected]> > 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 <[email protected]> > 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 <[email protected]> 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 > >>>> > >>> <[email protected] > >>> > >>>> > >>>>> wrote: > >>>> > >>>> On Mon, Jan 9, 2017 at 3:49 AM, Aljoscha Krettek <[email protected] > > > >>>>> 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 <[email protected]> > >>>>>> > >>>>> 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 > >>>>>>> > >>>>>> <[email protected]> > >>>>> > >>>>>> wrote: > >>>>>>> > >>>>>>> On Thu, Jan 5, 2017 at 9:28 AM, Jesse Anderson < > >>>>>>>> > >>>>>>> [email protected] > >>>> > >>>>> > >>>>>> 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 > >>>>>>>>> > >>>>>>>> <[email protected] > >>>>>>> > >>>>>>>> > >>>>>>>>> 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 < > >>>>>>>>>> > >>>>>>>>> [email protected]> > >>>>>>> > >>>>>>>> wrote: > >>>>>>>>>> > >>>>>>>>>> @Eugene, yes that failed on the checkstyle. > >>>>>>>>>>> > >>>>>>>>>>> On Wed, Jan 4, 2017 at 1:27 PM Eugene Kirpichov > >>>>>>>>>>> <[email protected]> wrote: > >>>>>>>>>>> > >>>>>>>>>>> Try just -Prelease. > >>>>>>>>>>>> On Wed, Jan 4, 2017 at 1:21 PM Jesse Anderson < > >>>>>>>>>>>> > >>>>>>>>>>> [email protected] > >>>>>>>> > >>>>>>>>> > >>>>>>>>>> wrote: > >>>>>>>>>>>> > >>>>>>>>>>>> Fails because I don't have a secret key. > >>>>>>>>>>>>> > >>>>>>>>>>>>> On Wed, Jan 4, 2017 at 1:03 PM Jean-Baptiste Onofré < > >>>>>>>>>>>>> > >>>>>>>>>>>> [email protected] > >>>>>>>>> > >>>>>>>>>> > >>>>>>>>>>> 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é > >>>>>>>>>>>>>> [email protected] > >>>>>>>>>>>>>> http://blog.nanthrax.net > >>>>>>>>>>>>>> Talend - http://www.talend.com > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>> > >>>> > >>> > >> > > -- > > Jean-Baptiste Onofré > > [email protected] > > http://blog.nanthrax.net > > Talend - http://www.talend.com > > > -- ------- Jason Kuster Apache Beam (Incubating) / Google Cloud Dataflow
