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

add possibility to set JPDA_SUSPEND via ENV var #1180

Merged
merged 9 commits into from
Nov 17, 2016
Merged

add possibility to set JPDA_SUSPEND via ENV var #1180

merged 9 commits into from
Nov 17, 2016

Conversation

riuvshin
Copy link

@riuvshin riuvshin commented Nov 17, 2016 •
edited

needed for: #1151

added possiblity to enable jpda suspend mode and change debug port

@garagatyi @TylerJewell please review

@TylerJewell
Copy link
Contributor

Add improvement to docs/README.md in sections where we discuss development mode

@riuvshin
Copy link
Author

@TylerJewell please review docs changes

benoitf
benoitf reviewed Nov 17, 2016
View reviewed changes
dockerfiles/cli/cli.sh
@@ -687,6 +687,7 @@ generate_configuration_with_puppet() {
-e "REGISTRY_ENV_FILE=${REGISTRY_ENV_FILE}" \
-e "POSTGRES_ENV_FILE=${POSTGRES_ENV_FILE}" \
-e "CODENVY_ENV_FILE=${CODENVY_ENV_FILE}" \
-e "ENABLE_JPDA_SUSPEND=${ENABLE_JPDA_SUSPEND:-false}" \
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks strange that we need to set extra env value there It's not in codenvy.env ?

@benoitf
Copy link
Contributor

benoitf commented Nov 17, 2016 •
edited

it looks strange that property is not having any prefix. It's JPDA debug for which container ?

properties like
CODENVY_DEBUG_MODE=true
CODENVY_DEBUG_SUSPEND=true
are more descriptive

then it gives JPDA option to tomcat (but if you replace tomcat by jetty, option will stay the same for user)

@benoitf
Copy link
Contributor

benoitf commented Nov 17, 2016 •
edited

example of properties in che launcher:


CHE_DEBUG_SERVER="false"
CHE_DEBUG_SERVER_PORT="8000"
CHE_DEBUG_SERVER_SUSPEND="false"

@riuvshin
Copy link
Author

@benoitf renamed env var to CODENVY_DEBUG_SUSPEND

@benoitf
Copy link
Contributor

benoitf commented Nov 17, 2016

I still don't get why
-e "CODENVY_DEBUG_SUSPEND=${CODENVY_DEBUG_SUSPEND:-false}" \
is given to the CLI directly.
if each parameter is added like that it will unmaintainable, no ?

I only see parameter in codenvy.env or/and in docker-compose files
In dev mode, you could set ENV var locally and then call docker-compose (native, not containerized)
or you can also tweak codenvy.env before updating the conf.

@riuvshin
Copy link
Author

Good point, I thought it is very rare use case when you need that suspend so I don't wanted to add this codenvy.env. But in general I kind of agree with you having different flow for setting configurations is not good. So I will move this to codenvy.env I think.

@riuvshin
Copy link
Author

riuvshin commented Nov 17, 2016 •
edited

ah, I forget that I've decided to enable it this way because "dev mode" enable also via mount in CLI, so maybe it still consistent... @TylerJewell @garagatyi WDYT guys?

@benoitf
Copy link
Contributor

benoitf commented Nov 17, 2016

It's not consistent at all
if you want to change debug port, you will not give another parameter to CLI directly as well

@TylerJewell
Copy link
Contributor

Do we really need to let people to change the debug port? It sounds like a nice option, but it may add complexity for a feature that <.0001% of people would use.

benoitf
benoitf suggested changes Nov 17, 2016
View reviewed changes
Copy link
Contributor

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

it's not matching maintainability and extensiblity

@benoitf
Copy link
Contributor

benoitf commented Nov 17, 2016

it's not because I'm using a local binary that I also want that my server run in debug mode as well.
debug port was only an example .
If any development mode option is becoming a new direct parameter it's not working

@riuvshin
Copy link
Author

riuvshin commented Nov 17, 2016 •
edited

I reworked everything now it is configurable in codenvy.env wihtout CLI modifications
docs updated also.
I've also added possibility to change debug port
@benoitf @TylerJewell @garagatyi please review

benoitf
benoitf reviewed Nov 17, 2016
View reviewed changes
modules/codenvy/templates/codenvy.env.erb
@@ -10,14 +10,16 @@ PUPPET_SOURCE=<%= scope.lookupvar('codenvy::puppet_src_folder') -%>
PUPPET_DESTINATION=<%= scope.lookupvar('codenvy::codenvy_folder') -%>

PGUSER=<%= scope.lookupvar('codenvy::pgsql_username') %>
PGPASSWORD=<%= scope.lookupvar('codenvy::pgsql_pass') %>
PGPASSWORD=<%= scope.lookupvar('codenvy::pgsql_pass') -%>
Copy link
Contributor

@benoitf benoitf Nov 17, 2016 •
edited

Choose a reason for hiding this comment

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

is - a typo ? because else on the previous line PG we don't have -%>

Copy link
Author

Choose a reason for hiding this comment

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

no it is a ruby thing it will prevent of new line generation

@benoitf
Copy link
Contributor

benoitf commented Nov 17, 2016

you could optionally reword description of the PR as example is not up-to-date

Updated
091311d 
1. Removed no longer respected dev_mode and dev_tomcat properties
2. Added better docs
@TylerJewell
Copy link
Contributor

I updated the documentation in the che.env file:

  1. The DEVELOPMENT_MODE property is no longer required as it is set in the CLI based upon :/repo volume mount.
  2. Same for the TOMCAT reference.
  3. Provided deeper description and formatting for JPDA mode.

@riuvshin
Copy link
Author

riuvshin commented Nov 17, 2016 •
edited

@TylerJewell no, we must have CODENVY_ENVIRONMENT ENV property because it is used in puppet CLI should set it if mount is provided. but without that property puppet will not be able to know that dev mode is on. And I think same for tomcat reference

@TylerJewell
Copy link
Contributor

Yes you are right. We need one but not the other. I will revert that one change.

@TylerJewell
Copy link
Contributor

Fixed

@riuvshin riuvshin merged commit c4b41b0 into master Nov 17, 2016
@riuvshin riuvshin deleted the codenvy-1151 branch November 17, 2016 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TylerJewell TylerJewell TylerJewell approved these changes

@benoitf benoitf benoitf approved these changes

@garagatyi garagatyi garagatyi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@riuvshin @TylerJewell @benoitf @garagatyi