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

Go back to previous behavior regarding Surefire and Maven IT #11037

Merged
merged 1 commit into from
Jul 28, 2020

Conversation

gsmet
Copy link
Member

@gsmet gsmet commented Jul 28, 2020

Follow up of #10919

We better get back to the previous behavior which was to never run the plugin rather than introducing some change in this area.

Remember when I said this whole native surefire skip thing was a bad idea?

@gsmet gsmet added this to the 1.7.0 - master milestone Jul 28, 2020
@famod
Copy link
Member

famod commented Jul 28, 2020

Trying to make sense of it (and learn something): What did I break exactly?

@gsmet
Copy link
Member Author

gsmet commented Jul 28, 2020

So you didn't break something, it's just that the original intent (the part you removed) was to always disable the surefire tests. And the new part you kept can have them enabled. I just got back to the original intent.

You had one chance out of two :).

@famod
Copy link
Member

famod commented Jul 28, 2020

Ok, understood.

I did not follow the PR in detail that introduced ${native.surefire.skip} but I knew that it was merged before, so the block I removed felt like a leftover. 🤷‍♂️

It would certainly be helpful if there was a short comment right over or next to this line clarifying the intent (since it not appears to be clear to everyone). This might help to prevent the next thoughtless bypasser from making the same mistake. 😉

@gsmet
Copy link
Member Author

gsmet commented Jul 28, 2020

TBH, I have no idea why it was there. I just don't think we should change the behavior with something that is orthogonal.

(and I won't spend more time on this as I was against the initial patch)

@gsmet gsmet merged commit 2aeae84 into quarkusio:master Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants