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

WFLY-3882 ManagementClient Domain Hosts #5

Closed
wants to merge 3 commits into from

Conversation

panossot
Copy link

Addition of a function at ManagementClient that returns the hosts that are included in the domain.

@@ -229,6 +231,22 @@ public boolean isServerStarted(Server server) {
return false;
}

public HashSet<String> getDomainHosts() throws Exception{
Copy link
Contributor

Choose a reason for hiding this comment

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

Return a Set<String> instead of HashSet<String>.

@bstansberry
Copy link
Contributor

Why does this belong in the arquillian container versus in some utility code in a testsuite?

All the other public API in this class is used within this project itself to help Arquillian manage a domain.

Also, what's the use case for this? The JIRA trail leads back to https://issues.jboss.org/browse/WFLY-1264 but I don't see how this provides any sort of notification capability.

readRootNode();

if (rootNode.isDefined())
if (rootNode.get("host").isDefined()){
Copy link
Contributor

Choose a reason for hiding this comment

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

Always use this idiom for this kind of thing:

if (rootNode.hasDefined("host"))

Never call ModelNode.get unless you are sure what you are getting exists, or you are willing to create the node. ModelNode provides a fluent API, so one effect of that is get(...) actuall mutates the node you call it on.

@panossot
Copy link
Author

panossot commented Oct 2, 2014

I have placed this modification in the ManagementClient because in the description of WFLY-1264, ManagementClient is to be used (Extention of functionality of ManagementClient). I will also add a PR to WFLY-1264 of the test which will display the notifications.

@panossot
Copy link
Author

panossot commented Oct 6, 2014

PR for WFLY-1264 sent : wildfly/wildfly#6781

@bstansberry
Copy link
Contributor

This isn't a notification mechanism, so it's not clear to me how this meets the WFLY-1264 requirements. This work needs to start with a full analysis in WFLY-1264 of what the requirements are and what the use cases are to support those requirements.

I'm going to close this pending that analysis.

@bstansberry bstansberry closed this Oct 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants