Deploy command created. #57
Conversation
|
An option -- CLI has |
|
Yes, you are right, I meant #42 |
|
I wanted to use management operation directly to prevent need of creating temporary files and to just leave it in memory. |
| /** | ||
| * This test uses the {@code ajp} socket binding for all listener types, because there's no listener configured | ||
| * by default that uses it (unlike {@code http}). | ||
| */ |
| private final boolean autoCloseInputStream; | ||
| private final List<String> serverGroups; | ||
|
|
||
| private Deploy(InputStream deploymentInputStream, Data deployCmdOptions) { |
There was a problem hiding this comment.
This is slightly worrying, because InputStream is a one-off thing. If we were on Java 8, I'd prefer Supplier<InputStream>. Another option would be Guava's ByteSource, and even though we already depend on Guava, I'd like to avoid depending on it in public APIs. Another very similar thing is ShrinkWrap's Asset. Sadly, on Java 6, there's probably nothing we can do (short of providing our own interface, which I'd really like to avoid) :-(
There was a problem hiding this comment.
Agree with you, don't now about any better way with Java 6 than using the InputStream. If that would be big issue, I would redo this and allow only File. In that case only in memory handling would be lost :-(.
There was a problem hiding this comment.
No need to do that, we can live with InputStream for now.
|
What are your plans with this? Do you want this reviewed and merged, or do you plan to work on undeploy commands first and prefer to keep this PR open? |
|
I plan to work on undeploy command, I believe I should have time to do it during this week. If no one is in hurry, I would wait for it :-). |
|
I have added also the undeploy command, now it fixes #42 as whole :-). Note I was thinking also about removing all options from undeploy command, that it would in case of domain always undeploy the deployment from all relevant server groups. As the way the undeploy command behaves, it requires using enabling option for keeping content if there wasn't all relevant server groups selected. WDYT? |
|
I've decided to remove the option for specifying server groups as I believe no one really wants to use it this way. If it is found needed, I left it as builder => it should be easy to add those options without breaking API. |
|
=> ready for review |
| /** | ||
| * A single shared instance of this class is passed through the entire builder invocation chain to accumulate | ||
| * all the options. | ||
| */ |
There was a problem hiding this comment.
This is clearly copied from Online|OfflineOptions, where I deliberately broke the builder to a bunch of classes to be able to provide compile-time checking of all mandatory options. It adds quite some maintenance burden, so I don't think it's suitable for commands. Did you have any special reason to use this pattern instead of plain old single-class builder?
There was a problem hiding this comment.
I just wanted to have also check at compile time, but I agree it is not so important in this case and having written in javadoc which options are only for domain should be satisfactory.
|
BTW: there is also option to rewrite the |
|
If you feel like using |
|
I have incorporated your suggestions, should be ready for another round of review :-) |
|
Also please rebase, I just pushed #61 :-) Shouldn't affect this PR, you're not doing anything version-specific. I'd also like to do a release today, so if you don't rebase & squash soon, this will have to wait for another release. Waiting for review from @simkam and @ochaloup is not a prerequisite for merge, though. |
|
For me it's fine. I haven't found anything special that I would need for more from the API. |
| if (serverGroups != null) { | ||
| this.serverGroups.addAll(Arrays.asList(serverGroups)); | ||
| } | ||
| return build(); |
There was a problem hiding this comment.
I talked to Radim and he mentioned that he is not sure if this ok. I agree that this could problem in future if we eventually add more options to builder. @Ladicek wdyt?
There was a problem hiding this comment.
It's also a bit unusual. I agree it would be better if these methods just returned Builder and the user always had to call build().
There was a problem hiding this comment.
On the other hand, as Radim just told me, it makes sure that the user can't call both toServerGroups and toAllServerGroups, which is nice. IMHO it would be enough to throw an exception when it's detected that the user called both of these methods, the question is how to detect this -- that would require slightly more complex encoding in the Builder.
Address.deployment added in the process.
|
I have done force update, where I have squashed the commits, rebased and modified the builder to return the builder object for |
|
OK, thanks. |
Adding command for adding application-security-domain into Undertow subsystem
Created deploy command, which handles deploy part of #42.
EDIT: updated link to point to correct issue.