Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BEAM-1513] Skip slower verifications if '-Dquick' specified. Enable them otherwise #2048

Closed
wants to merge 3 commits into from

Conversation

aviemzur
Copy link
Member

@aviemzur aviemzur commented Feb 19, 2017

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

  • Make sure the PR title is formatted like:
    [BEAM-<Jira issue #>] Description of pull request
  • Make sure tests pass via mvn clean verify. (Even better, enable
    Travis-CI on your fork and ensure the whole test matrix passes).
  • Replace <Jira issue #> in the title with the actual Jira issue
    number, if there is one.
  • If this contribution is large, please file an Apache
    Individual Contributor License Agreement.

@aviemzur
Copy link
Member Author

R: @dhalperi

@jbonofre
Copy link
Member

CC: @jbonofre

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.003%) to 69.184% when pulling 86d42a6 on aviemzur:skip-slow-verifications into 2872f86 on apache:master.

@asfbot
Copy link

asfbot commented Feb 19, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7593/
--none--

@aviemzur aviemzur changed the title [BEAM-1513] Skip slower verifications if '-DskipTests' specified [BEAM-1513] Skip slower verifications if '-Dquick' specified. Enable them otherwise Feb 21, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 69.197% when pulling 0e4f13a on aviemzur:skip-slow-verifications into 8cfb3d1 on apache:master.

@asfbot
Copy link

asfbot commented Feb 21, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7615/
--none--

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 69.18% when pulling 0e4f13a on aviemzur:skip-slow-verifications into 8cfb3d1 on apache:master.

@asfbot
Copy link

asfbot commented Feb 21, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7618/
--none--

@jbonofre
Copy link
Member

Thanks for the change. I'm reviewing.

@@ -48,24 +48,23 @@
</profile>
<profile>
<id>release</id>
<activation>
<property>
<name>!quick</name>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not condition release profile with quick property. Imagine if the release manager does mvn release:prepare -Dquick it would not be good.
Further more, the user still have to do mvn clean install -Prelease to enable all verification right ? I would activate all verifications by default and disable only with -Dquick.

Copy link
Member Author

@aviemzur aviemzur Feb 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone performing a release will specify -Dquick wouldn't that be human error? The same is true if they would specify any other flags which they should not.
Regarding the second question, this change indeed makes the verifications (checkstyle, rat, findbugs) enabled by default and disabled only with -Dquick.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.

@jbonofre
Copy link
Member

I'm just testing a new time.

@aviemzur
Copy link
Member Author

Rebased on master

@jbonofre
Copy link
Member

Thanks. Updating and merging.

@asfbot
Copy link

asfbot commented Feb 21, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7641/
--none--

Copy link
Member

@jbonofre jbonofre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Merging.

@jbonofre
Copy link
Member

For the record, I would mention -Dquick in the README.md and also propose a pull request for website.

@asfgit asfgit closed this in 3fcc820 Feb 21, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 69.175% when pulling 3f01de1 on aviemzur:skip-slow-verifications into ebd4ae6 on apache:master.

@asfbot
Copy link

asfbot commented Feb 21, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7646/
--none--

@davorbonaci
Copy link
Member

Hold on -- this was sent to @dhalperi for review and was merged before he had a chance to take a look.

@jbonofre, @aviemzur, can we please reset this?

@dhalperi, I'd kindly ask to review according to the resolution on the mailing list and let's address comments in the followup pull request please? Thanks!

@dhalperi
Copy link
Contributor

dhalperi commented Feb 21, 2017

I read this thread https://lists.apache.org/thread.html/48383f94a0ec3f5bae6276d77f9826ee7676ac24273e3b74ec792fe1@%3Cdev.beam.apache.org%3E as building a consensus that checks should be off by default, and relesae profile should be opt in. It seems like the following people were in favor of that:

and the following people wanted to change as this proposal:

That I believe is everyone who rang into the specific discussion on-or-around Feb 10.

So in other words, I think this PR is mostly against the will of the community.

@jbonofre
Copy link
Member

Sorry about that guys, I thought we got a consensus on the mailing list about -Dquick.

@dhalperi @aviemzur agree to revert ?

Again, my apologize, I thought we were OK.

@aviemzur
Copy link
Member Author

Sure, if the majority is against, let's revert.
I wanted the PR up for review to see if we want to use it, as I believe it is a good middle ground that most could live with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants