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 > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > >
