+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
