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

ARQ-2071 Use more safe serverifo command to check if container runs #37

Merged
merged 2 commits into from
Feb 8, 2017

Conversation

vjuranek
Copy link
Contributor

@vjuranek vjuranek commented Feb 2, 2017

Short description of what this resolves:

Use more safe serverifo command to check if container runs

Changes proposed in this pull request:

  • Switches command for checking if container runs from list to serverinfo as not all session managers implements list command (typically for security reasons)

Fixes: https://issues.jboss.org/browse/ARQ-2071

public void serverInfo() throws IOException {
execute(tomcatManagerCommandSpec.getServerInfoCommand(), null, null, -1);
}

public void list() throws IOException {
Copy link
Member

@bartoszmajsak bartoszmajsak Feb 8, 2017

Choose a reason for hiding this comment

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

It seems that by introducing serverInfo list method became obsolete. Can you clean up unused code?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's same also for the methods getListCommand() in Tomcat**ManagerCommandSpec classes

@vjuranek
Copy link
Contributor Author

vjuranek commented Feb 8, 2017

As discussed with @MatousJobanek, to keep backward compatibility, list() method should be deprecated first and removed in release after next release. I added deprecated annotation and comment. Is this ok?

@bartoszmajsak
Copy link
Member

Is it something which is public facing? I thought it's more an internal implementation of how we handle the containers (in this case Tomcat). If that is the case, then obviously @Deprecated makes more sense.

@vjuranek
Copy link
Contributor Author

vjuranek commented Feb 8, 2017

The method is public, so yes, it's public. I can imagine that someone who has some custom extension can rely on this method, but as I'm not very familiar with Arquillian project and its community, project policy etc., I of course can remove it if you like (I just wanted to pop this up for consideration as this is what many projects do - mark public method deprecated in the release after the change and remove it the next release).

@bartoszmajsak
Copy link
Member

I totally understand your concern and appreciate raising it. I'm not aware of any usage of it. I thought maybe @MatousJobanek knows something :)

@MatousJobanek
Copy link
Contributor

Neither do I TBH :-D. I thought that @vjuranek uses this code somewhere in his test suite (as an API method).
It's really funny this conversation - too many assumptions. We should follow @bartoszmajsak and not to have any expectation and don't assume anything :-D

On the other hand, we have a CR release and according to "standard process" I think that we shouldn't remove (radically change) anything that is publicly available (public methods mainly in interfaces).

Personally, I would keep it with the annotation @Deprecated for this time, release finally the Final version and then remove it completely from the code.

@bartoszmajsak
Copy link
Member

+1, squashed and merged! Thank you @vjuranek

@bartoszmajsak bartoszmajsak reopened this Feb 8, 2017
@bartoszmajsak bartoszmajsak merged commit 67bf420 into arquillian:master Feb 8, 2017
@vjuranek
Copy link
Contributor Author

vjuranek commented Feb 8, 2017

great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants