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

configures the environment with the context paths before the application.run() #1503

Merged
merged 1 commit into from Mar 31, 2016

Conversation

Projects
None yet
4 participants
@qinfchen
Contributor

qinfchen commented Mar 31, 2016

This commit configures the context paths for the application before the
application run() instead of setting it prior to starting the server.
This allows applications to obtain the context paths during the run
method.

This shouldn't break any changes to the users, but note that
ServerFactory now needs to call configure explicitly to set the
context paths in the environment

configures the context paths before the application.run()
This commit configures the context paths for the application before the
application `run()` instead of setting it prior to starting the server.
This allows applications to obtain the context paths during in the run
method.

This shouldn't break any changes to the users, but note that
ServerFactory now needs to call `configure` explicitly to set the
context paths in the environment

@arteam arteam merged commit 6cc53f0 into dropwizard:master Mar 31, 2016

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Mar 31, 2016

Member

Looks good! It seems that this improvement doesn't break anything and provides a way to get access to context paths in an application class. I remember some users asked for this feature in the mailing list.

Thank you very much for your contribution!

Member

arteam commented Mar 31, 2016

Looks good! It seems that this improvement doesn't break anything and provides a way to get access to context paths in an application class. I remember some users asked for this feature in the mailing list.

Thank you very much for your contribution!

@arteam arteam added the improvement label Mar 31, 2016

@arteam arteam added this to the 1.0.0 milestone Mar 31, 2016

arteam added a commit that referenced this pull request Mar 31, 2016

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Mar 31, 2016

Member

Oh, for some reasons the build fails on Travis after merging. I've reverted the commit for the being.
Will try to try find a source of the error.

Member

arteam commented Mar 31, 2016

Oh, for some reasons the build fails on Travis after merging. I've reverted the commit for the being.
Will try to try find a source of the error.

@patrox

This comment has been minimized.

Show comment
Hide comment
@patrox

patrox Mar 31, 2016

Contributor

@arteam shouldn't this commit be reverted as well: 171bbe6 ?

Contributor

patrox commented Mar 31, 2016

@arteam shouldn't this commit be reverted as well: 171bbe6 ?

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Mar 31, 2016

Member

@patrox Let's hope we will find the error cause and this improvement will be merged anyway :)

Member

arteam commented Mar 31, 2016

@patrox Let's hope we will find the error cause and this improvement will be merged anyway :)

@@ -136,4 +132,13 @@ public Server build(Environment environment) {
return server;
}
@Override
public void configure(Environment environment) {

This comment has been minimized.

@jplock

jplock Mar 31, 2016

Member

since this is the same in both SimpleServerFactory and DefaultServerFactory, should we move this to the AbstractServerFactory instead?

@jplock

jplock Mar 31, 2016

Member

since this is the same in both SimpleServerFactory and DefaultServerFactory, should we move this to the AbstractServerFactory instead?

This comment has been minimized.

@qinfchen

qinfchen Mar 31, 2016

Contributor

the context path logic is slightly different between the two. It would be nice to move them into AbstractServerFactory

@qinfchen

qinfchen Mar 31, 2016

Contributor

the context path logic is slightly different between the two. It would be nice to move them into AbstractServerFactory

@qinfchen

This comment has been minimized.

Show comment
Hide comment
@qinfchen

qinfchen Mar 31, 2016

Contributor

@arteam found the issue in the original test. configure needs to be called to setup the environment for other tests. Separate PR is created. Sorry about that

Contributor

qinfchen commented Mar 31, 2016

@arteam found the issue in the original test. configure needs to be called to setup the environment for other tests. Separate PR is created. Sorry about that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment