PAYARA-2113 Made stop-domain a little more descriptive #2169
Conversation
|
jenkins test please |
|
Quick build and test failed! |
|
jenkins test please |
|
Quick build and test passed! |
michaelranaldo
force-pushed
the
PAYARA-2113-Misleading-Message-For-Stop-Domain-If-Domain1-Not-Running
branch
2 times, most recently
from
deece11 to
64f148e
Compare
…more on HK2 magic
michaelranaldo
force-pushed
the
PAYARA-2113-Misleading-Message-For-Stop-Domain-If-Domain1-Not-Running
branch
from
64f148e to
5149308
Compare
|
jenkins test please |
|
Quick build and test passed! |
| throw new CommandException(ex.getLocalizedMessage()); | ||
| } | ||
| return 0; | ||
| } | ||
|
|
||
| protected List<String> getRunningDomains() throws IOException, DomainException, CommandException { |
There was a problem hiding this comment.
I would probably have this method return something more generic. Maybe a Map<String, String> mapping domains to their status or something similar. IMHO, the list-domains command shouldn't care about whether the domain is running or not. It's just me being pedantic, but I think it's better to invert the control on this one
There was a problem hiding this comment.
Right, makes sense. I shall bow to the pedantry in this case ;)
|
jenkins test please |
|
Quick build and test passed! |
| @@ -65,7 +65,7 @@ | |||
| @PerLookup | |||
| public final class ListDomainsCommand extends LocalDomainCommand { | |||
|
|
|||
| private static final LocalStringsImpl strings = new LocalStringsImpl(ListDomainsCommand.class); | |||
| private static final LocalStringsImpl Strings = new LocalStringsImpl(ListDomainsCommand.class); | |||
There was a problem hiding this comment.
This should follow naming conventions of all capitals
| } | ||
| } catch(Exception e) { | ||
| } | ||
| runningDomains = | ||
| (runningDomains.length() < 1) | ||
| ? "\nNo domains are currently running." |
There was a problem hiding this comment.
I like this change, but could it be implemented using a different message name in the LocalString.properties?
| return 0; | ||
| } | ||
| // If it has gotten this far, the domain has failed to be stopped so the command has failed | ||
| return 1; |
There was a problem hiding this comment.
Why have you changed the return value for the command?
There was a problem hiding this comment.
At this point the command has failed to find a valid running instance that matches the input, so it would make more sense to have the command report it as a failure.
|
jenkins test please |
|
Quick build and test passed! |
|
HK2 cannot be used as it may be run when there is no running server and so it does not have access to HK2. |
|
Sounds like something that should be done in a separate PR |
|
I believe Jonathan's comment is pertinent to the PR. If HK2 can't be used, the current code won't work. I believe it works just fine, but it does need clarifying |
|
Interesting; definitely something worth investigating. |
…ding-Message-For-Stop-Domain-If-Domain1-Not-Running PAYARA-2113 Made stop-domain a little more descriptive
…essage-For-Stop-Domain-If-Domain1-Not-Running (#2197) PAYARA-2113 Made stop-domain a little more descriptive
…ding-Message-For-Stop-Domain-If-Domain1-Not-Running PAYARA-2113 Made stop-domain a little more descriptive
…ain-If-Domain1-Not-Running-fs (pull request payara#167) Merge pull request payara#2169 from michaelranaldo/PAYARA-2113-Misleading-Message-For-Stop-Domain-If-Domain1-Not-Running
…3-Misleading-Message-For-Stop-Domain-If-Domain1-Not-Running (pull request payara#167)" This reverts pull request payara#167. > PAYARA-2113 Made stop-domain a little more descriptive
Revert "Merge pull request payara#2169 from michaelranaldo/PAYARA-2113-Misleading-Message-For-Stop-Domain-If-Domain1-Not-Running (pull request payara#167)"
Addresses #1841