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

[WFCORE-4412] Eliminating CapabilityServiceTarget.addCapability() deprecated method usages #3735

Merged
merged 2 commits into from Apr 17, 2019

Conversation

ropalka
Copy link
Contributor

@ropalka ropalka commented Apr 9, 2019

@wildfly-ci wildfly-ci added the deps-ok Dependencies have been checked, and there are no significant changes label Apr 9, 2019
Copy link
Contributor

@darranl darranl left a comment

Choose a reason for hiding this comment

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

I think the Elytron refactoring needs removing from this PR - technically that commit should also have a Jira reference anyway but we have a separate PR being prepared that is looking to reduce the startup mode of a number of services #3729

@ropalka
Copy link
Contributor Author

ropalka commented Apr 10, 2019

The commit was excluded as you suggested @darranl

@darranl darranl dismissed their stale review April 10, 2019 16:01

Elytron commits removed.

@ropalka
Copy link
Contributor Author

ropalka commented Apr 11, 2019

Retest this please

@wildfly-ci
Copy link

Core - Full Integration Build 8504 outcome was FAILURE using a merge of 75df45d
Summary: Tests failed: 1 (1 new), passed: 4851, ignored: 134 Build time: 02:03:36

Failed tests

org.jboss.as.test.integration.messaging.mgmt.JMSTopicManagementTestCase.testCountMessagesForSubscription: java.lang.AssertionError: expected:<2> but was:<0>
	at org.jboss.as.test.integration.messaging.mgmt.JMSTopicManagementTestCase.testCountMessagesForSubscription(JMSTopicManagementTestCase.java:231)


@ropalka
Copy link
Contributor Author

ropalka commented Apr 12, 2019

Retest this please

@darranl
Copy link
Contributor

darranl commented Apr 12, 2019

@ropalka please don't use the test phrase unless you see a lot of errors as you kick off loads of jobs on CI that already passed. Instead click on the details link and you can click run at the top right of the results screen to kick off the specific job again.

@ropalka
Copy link
Contributor Author

ropalka commented Apr 12, 2019

Thanks for the hint @darranl - will do it since now on.

Copy link
Member

@jmesnil jmesnil left a comment

Choose a reason for hiding this comment

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

Looks good to me (except my question about elytron's TrivialAddHandler)

@@ -58,12 +58,14 @@
this.initialMode = checkNotNullParam("initialMode", initialMode);
Copy link
Member

Choose a reason for hiding this comment

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

@darranl @ropalka Is it ok to keep this elytron change or should it be handled by another Elytron PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jmesnil this current change looks Ok to me, the issue was previously @ropalka was removing an 'unused' method that coincidentally a second PR was about to start using.

@jmesnil jmesnil added the ready-for-merge This PR is ready to be merged and fulfills all requirements label Apr 16, 2019
@jmesnil jmesnil merged commit 2535d66 into wildfly:master Apr 17, 2019
@ropalka ropalka deleted the WFCORE-4412 branch April 17, 2019 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps-ok Dependencies have been checked, and there are no significant changes ready-for-merge This PR is ready to be merged and fulfills all requirements
Projects
None yet
4 participants