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.

Sign up for GitHub

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

Jump to bottom

DRYness: consolidate duplicated code in extension build plugins #28688

Merged
merged 1 commit into from
Oct 25, 2022
Merged

DRYness: consolidate duplicated code in extension build plugins #28688

merged 1 commit into from
Oct 25, 2022

Conversation

holly-cummins
Copy link
Contributor

I've added a dependency on devtools-common from all of the extension build plugins, so that I can eliminate duplicated code I introduced in the last changeset.

The implementation and the test method are just copied across from the duplicated versions.

@quarkus-bot quarkus-bot bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/gradle Gradle area/maven area/platform Issues related to definition and interaction with Quarkus Platform labels Oct 19, 2022
@holly-cummins holly-cummins mentioned this pull request Oct 19, 2022
Merged
@quarkus-bot

This comment has been minimized.

Sign in to view
@holly-cummins holly-cummins marked this pull request as draft October 19, 2022 16:56
@holly-cummins
Copy link
Contributor Author

Scratch that about just copying and pasting. I've copy and pasted the implementation, but I made an error in the common-tools test setup that only showed in the github actions, so I've switched to System Stubs for controlling the environment variables in tests.

@holly-cummins holly-cummins marked this pull request as ready for review October 19, 2022 17:12
@holly-cummins
Copy link
Contributor Author

Scratch that about just copying and pasting. I've copy and pasted the implementation, but I made an error in the common-tools test setup that only showed in the github actions, so I've switched to System Stubs for controlling the environment variables in tests.

@holly-cummins
Copy link
Contributor Author

@aloubyansky, do you mind looking at this?

@aloubyansky
Copy link
Member

Let's abandon the quarkus-bootstrap-maven-plugin. If we didn't have an aggregating root pom.xml, it would fail the build due to a circular dependency between the bootstrap project and the tools, those are supposed to independent projects.

@holly-cummins
Copy link
Contributor Author

Let's abandon the quarkus-bootstrap-maven-plugin. If we didn't have an aggregating root pom.xml, it would fail the build due to a circular dependency between the bootstrap project and the tools, those are supposed to independent projects.

@aloubyansky do you mean "remove it from the product" or "go back to the WET implementation without the circular dependency on that module" or "not add the scm information in that module?"

When I was looking I noticed that there's definitely some extensions in the quarkiverse using that one.... but it is adding a maintenance overhead now.

@aloubyansky
Copy link
Member

I meant to not keep enhancing it. Projects that are still using it should have warnings in their build logs. We could probably open PRs for those that we know about.
I probably wouldn't remove it yet.

@holly-cummins
Copy link
Contributor Author

I meant to not keep enhancing it. Projects that are still using it should have warnings in their build logs. We could probably open PRs for those that we know about. I probably wouldn't remove it yet.

I did think removing it sounded drastic, but some teams move fast. :)

After this PR, I think all scm-related future enhancements in it will be passive, because the brains will be in the common code. Or do you think I should take it out of this PR?

@aloubyansky
Copy link
Member

We don't want to introduce a circular dependency between the tools and bootstrap. I meant to leave the bootstrap plugin out of this change.

@holly-cummins holly-cummins force-pushed the fix-duplication-for-scm-metadata branch from 06f553b to e6f40aa Compare October 21, 2022 17:59
@holly-cummins
Copy link
Contributor Author

Got it, thanks! In my first version of the changes I did introduce a new module, devtools-extensions to avoid the circular dependency and other dependency badness, but then I decided it was too much clutter. I've removed bootstrap from the changes, so it will have the original support and not get any 'reading the pom' or 'adding the tag' enhancements. I think that's fine, given it's deprecated.

@quarkus-bot

This comment has been minimized.

Sign in to view
@holly-cummins
DRYness: consolidate duplicated code
dafa8d2 
I've added a dependency on devtools-common from all of the extension build plugins, so that I can eliminate duplicated code I introduced in the last changeset.

The implementation is just copied across from the duplicated versions. I rewrote the test to use webcompere system stubs, for more control and more consistent behaviour. (And avoiding irritating CI-only failures.)

I left bootstrap out of the changeset to avoid a circular dependency. Since I removed the changes to the bootstrap bom, which is the parent of the extension-maven-plugin, so I need to set the version manually for that plugin.
@holly-cummins holly-cummins force-pushed the fix-duplication-for-scm-metadata branch from e6f40aa to dafa8d2 Compare October 21, 2022 18:09
@quarkus-bot
Copy link

quarkus-bot bot commented Oct 21, 2022

Failing Jobs - Building dafa8d2

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
✖ JVM Tests - JDK 17 MacOS M1 Build Failures Logs Raw logs
✔️ JVM Tests - JDK 18

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 MacOS M1 #

- Failing: integration-tests/mongodb-devservices

📦 integration-tests/mongodb-devservices

✖ io.quarkus.it.mongodb.BookResourceTest.health line 45 - More details - Source on GitHub

java.net.SocketTimeoutException: Read timed out
	at java.base/sun.nio.ch.NioSocketImpl.timedRead(NioSocketImpl.java:283)
	at java.base/sun.nio.ch.NioSocketImpl.implRead(NioSocketImpl.java:309)

✖ io.quarkus.it.mongodb.BookResourceTest.testReactiveClients line 39 - More details - Source on GitHub

java.net.SocketTimeoutException: Read timed out
	at java.base/sun.nio.ch.NioSocketImpl.timedRead(NioSocketImpl.java:283)
	at java.base/sun.nio.ch.NioSocketImpl.implRead(NioSocketImpl.java:309)

✖ io.quarkus.it.mongodb.BookResourceTest.testBlockingClient line 34 - More details - Source on GitHub

java.net.SocketTimeoutException: Read timed out
	at java.base/sun.nio.ch.NioSocketImpl.timedRead(NioSocketImpl.java:283)
	at java.base/sun.nio.ch.NioSocketImpl.implRead(NioSocketImpl.java:309)

✖ io.quarkus.it.mongodb.BookResourceWithParameterInjectionTest.testInjectedClient line 31 - More details - Source on GitHub

java.net.SocketTimeoutException: Read timed out
	at java.base/sun.nio.ch.NioSocketImpl.timedRead(NioSocketImpl.java:283)
	at java.base/sun.nio.ch.NioSocketImpl.implRead(NioSocketImpl.java:309)

✖ io.quarkus.it.mongodb.OtherProfileBookResourceTest.testBlockingClient line 14 - More details - Source on GitHub

java.net.SocketTimeoutException: Read timed out
	at java.base/sun.nio.ch.NioSocketImpl.timedRead(NioSocketImpl.java:283)
	at java.base/sun.nio.ch.NioSocketImpl.implRead(NioSocketImpl.java:309)

@geoand
Copy link
Contributor

geoand commented Oct 25, 2022

Is this one good to go? If so, please merge

@holly-cummins
Copy link
Contributor Author

This is good to go - the test failures are #28779.

@geoand
Copy link
Contributor

geoand commented Oct 25, 2022

Thanks.

I'll let @aloubyansky push the button

@aloubyansky aloubyansky merged commit 64db796 into quarkusio:main Oct 25, 2022
@quarkus-bot quarkus-bot bot added this to the 2.14 - main milestone Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aloubyansky aloubyansky aloubyansky approved these changes

Assignees
No one assigned
Labels
area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/gradle Gradle area/maven area/platform Issues related to definition and interaction with Quarkus Platform
Projects
None yet
Milestone
2.14.0.CR1
Development

Successfully merging this pull request may close these issues.
3 participants
@holly-cummins @aloubyansky @geoand