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.

Sign up for GitHub

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

Jump to bottom

Introduce a template-based custom docker server evaluation strategy #4928

Merged
merged 4 commits into from
May 12, 2017
Merged

Introduce a template-based custom docker server evaluation strategy #4928

merged 4 commits into from
May 12, 2017

Conversation

benoitf
Copy link
Contributor

@benoitf benoitf commented Apr 26, 2017 •
edited
Loading

What does this PR do?

This custom server evaluation strategy allows to configure the links by providing a set of macro/templates.
It avoids to create one strategy class file for each needs, as template/macros can be used independently.

What issues does this PR fix or reference?

#1560
#4361

Changelog

Introduce a template-based docker server evaluation stategy

Release Notes

Introduce a template-based docker server evaluation stategy

Docs PR

eclipse-che/che-docs#227

Change-Id: I3d941e0e786bdbfec037ba59e3f790f890bf9eb0
Signed-off-by: Florent BENOIT fbenoit@codenvy.com

@benoitf benoitf added kind/task Internal things, technical debt, and to-do tasks to be performed. sprint/current status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. labels Apr 26, 2017
@benoitf benoitf self-assigned this Apr 26, 2017
@codenvy-ci
Copy link

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/2519/

garagatyi
garagatyi reviewed Apr 28, 2017
View reviewed changes
Copy link

@garagatyi garagatyi left a comment

Choose a reason for hiding this comment

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

Though I'm considering the code a bit complicated it looks OK to me.
Can you address my line comments and take a look on code coverage of new class. Looks like there is plenty room for coverage improvement.

assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/codenvy/che.properties Outdated
# The 'docker-local' strategy may be useful if a firewall prevents communication between che-server and
# workspace containers, but will prevent communication when che-server and workspace containers are not
# on the same Docker network.
che.docker.server_evaluation_strategy=default


# Here are macros available for the custom server evaluation strategy
# serverName: name of the server exposing the port (like tomcat8, ws-agent, etc)
Copy link

Choose a reason for hiding this comment

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

I suppose we use server reference term for that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/codenvy/che.properties Outdated
# chePort : Che listening port number of workspace master
# wildcardNipDomain : get external address transformed into a nip.io DNS sub-domain
# wildcardXipDomain : get external address transformed into a xip.io DNS sub-domain
che.docker.custom.external.template=<serverName>.<machineName>.<workspaceId>.<wildcardNipDomain>:<chePort>
Copy link

Choose a reason for hiding this comment

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

Property name should say about it purpose. che.docker.custom is not clear what means custom. I would suggest che.docker.server_evaluation_strategy.custom instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated property names according to your remarks

@benoitf benoitf force-pushed the custom-docker-strategy branch from 20f242b to 61c6359 Compare May 9, 2017 15:32
@benoitf
Copy link
Contributor Author

benoitf commented May 9, 2017 •
edited
Loading

code coverage of the class is now 100%

@codenvy-ci
Copy link

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/2556/

@benoitf
Copy link
Contributor Author

benoitf commented May 11, 2017

@TylerJewell are you ok with the name of the properties ?

che.docker.server_evaluation_strategy.custom.template
che.docker.server_evaluation_strategy.custom.external.protocol

benoitf added 4 commits May 11, 2017 09:35
@benoitf
Introduce a custom server evaluation strategy
709a9d9 
This custom server evaluation strategy allows to configure the links by providing a set of macro/templates.
It avoids to create one strategy class file for each needs, as template/macros can be used independently.

Change-Id: I3d941e0e786bdbfec037ba59e3f790f890bf9eb0
Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>
@benoitf
Fix property names
4016363 
Change-Id: Ib2c145c5dc257480b0b31a0e5df17fe6f6a5d480
Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>
@benoitf
use lazy init
2d0ed83 
add more tests

Change-Id: I73576a61b1d47470352ece0531f8ac63ebfc755e
Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>
@benoitf
fixup! Introduce a custom server evaluation strategy This custom serv…
b4e633f 
…er evaluation strategy allows to configure the links by providing a set of macro/templates. It avoids to create one strategy class file for each needs, as template/macros can be used independently.

Change-Id: I2c8447255babb36749928bebe73a1f9e8cca4b52
Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>
@benoitf benoitf force-pushed the custom-docker-strategy branch from 61c6359 to b4e633f Compare May 11, 2017 07:35
@codenvy-ci
Copy link

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/2562/

@benoitf benoitf mentioned this pull request May 11, 2017
Closed
@skabashnyuk
Copy link
Contributor

Are there any changes in current URL format for the Eclipse Che?

@benoitf
Copy link
Contributor Author

benoitf commented May 11, 2017

@skabashnyuk I'm not sure to have fully understood the question.

You mean, if we don't use this strategy, is that URL to access che is changing ?
nothing change for this case

Or if by using that strategy, URLs to reach workspace agent, etc are changing ?
The reply is : yes it changes if you use a template that performs a change.
but you can define a template that could use another format

@benoitf benoitf removed the request for review from TylerJewell May 11, 2017 13:04
@benoitf benoitf requested a review from slemeur May 11, 2017 13:51
@benoitf benoitf added this to the 5.11.0 milestone May 11, 2017
@benoitf
Copy link
Contributor Author

benoitf commented May 12, 2017

Adding doc PR eclipse-che/che-docs#227

@slemeur
Copy link
Contributor

slemeur commented May 12, 2017

Thanks for the PR on docs !

@benoitf benoitf merged commit 52e418b into master May 12, 2017
@benoitf benoitf removed sprint/current status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. labels May 12, 2017
@benoitf benoitf deleted the custom-docker-strategy branch May 24, 2017 09:03
JPinkney pushed a commit to JPinkney/che that referenced this pull request Aug 17, 2017
@benoitf
Introduce a template-based custom docker server evaluation strategy (e…
b9ccf89 
…clipse-che#4928)

* Introduce a custom server evaluation strategy
This custom server evaluation strategy allows to configure the links by providing a set of macro/templates.
It avoids to create one strategy class file for each needs, as template/macros can be used independently.

Change-Id: I3d941e0e786bdbfec037ba59e3f790f890bf9eb0
Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garagatyi garagatyi garagatyi approved these changes

@slemeur slemeur slemeur approved these changes

@skabashnyuk skabashnyuk skabashnyuk approved these changes

Assignees

@benoitf benoitf

Labels
kind/task Internal things, technical debt, and to-do tasks to be performed.
Projects
None yet
Milestone
5.11.0
Development

Successfully merging this pull request may close these issues.
5 participants
@benoitf @codenvy-ci @skabashnyuk @slemeur @garagatyi