-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Mechanism to configure Che with configs from different sources #2175
Conversation
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/160/ |
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/162/ |
@andrew00x - this is amazing. How are you doing these days? I had to look at this issue a few times to my amazement. Super happy to see that it was this simple to implement for #2014. @vparfonov - I think there are changes in branch CHE-2116 that need to be removed if we accept these items. I had hard coded CHE_EXTRA_VOLUME_MOUNT environment variable. |
@vparfonov @skabashnyuk - I would like to go ahead and create a default Where should this file be located? Do we want to place it in the same location that we have our current default che.properties in the WEB-INF/ directory? |
There are a few changes that is like to see: |
|
|
|
So would that be:
? |
can be like this IMO
|
That would mean that alias.properties can only exist in one location, which is in our source code. What we do now, is that users can set a https://eclipse-che.readme.io/docs/usage-docker#environment-variables We should preserve the use of that variable and allow end users to provide alias overrides if they want. So the default would be based upon what we do now:
And if a user provides
|
Yes, I know that (I invented this behaviour ) ) and we can create same for aliases but in our opinion it is overkill. I do not think we should encourage user to do that, IMO it is exceptional workaround. |
I am ok to leave it as one time only. But please let's get this merged ASAP. We have many other issues dependent upon this. Andrew - just put it as a single file in web-INF. Also can you setup a default alias with the new names? |
Do you have a content for aliases file? |
Yes. In this issue, I have created new names for existing entries. There was one section where I hoped to have an array syntax but we can ignore that since we do not support that with properties I believe. |
I can't see a file with aliases in this issue. It have to be in format |
The file needs to be formatted but the data is there. Each entry has the new name as uncommented property and the existing names right below it in a comment. |
Right now I'm working on implementation of #2175 (comment) |
I've decided to postpone implementation of #2175 (comment) code looks overcomplicated and over-engineered. |
@skabashnyuk - I think gennady just said to have single alias.properties file in \WEB-INF directory only. So what is complicated here? |
The issues (not a big one but it can be challenging) we need individual files for each assembly. One for che, one for artik one for codenvy. It can't be reused if we put it in \web-INF |
Hummm... I thought we ignored *.properties for Codevny and that the puppet system was used instead? For ARTIK, it is a custom assembly, so you wouldn't that custom assembly just overwrite the WEB-INF\alias.properties file? |
So the issue is: if you want to reuse new property names across different assemblies aliases files have to be duplicated. Are you going to use new names only for Che? |
I've tested che with current state of @andrew00x PR and it's working ok. I'm proposing to merge it as is. And add aliases as part of #2015 |
Yeah, ok - to keep them separate. To be clear, where is the alias file located, and what is its name? With the long discussion, it's unclear where and what this file should be now. I can prepare one and submit PR for #2015. |
|
ok to merge |
Cannot start wsagent, multiple bindings of "api.endpoint": Tomcat logs:
List of environment variables in container:
|
According to steaktrace property is configured in MachineModule A binding to java.lang.String annotated with This is not expected, configuration might be overridden only it is On Tue, Aug 23, 2016 at 6:01 PM, Michail Kuznetsov <notifications@github.com
|
@skabashnyuk @gazarenkov - I think this is for you to decide? |
@andrew00x - can we st status on a fix? We cannot release some new features without this. |
@TylerJewell We are not able to start dev machines, We need some changes in our code. Right now we have no time for that so we postpone this issue. We will come back to it as soon as we will have some free time. |
This was needed before we launch Che in Che. So pmease assign to someone for this current or beginning of next sprint. |
Build # 231 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/231/ to view the results. |
Che machines has been started successfully |
…prefix of environment variables with "CHE_" prefix
Build # 245 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/245/ to view the results. |
Waiting #1818 to merge. |
…se-che#2175) * helper class for set and restore system properties * override configuration properties from system properties with "che." prefix of environment variables with "CHE_" prefix * fix bug report warnings, typo and avoid unneeded compilations of Pattern * Removed ApiEndpointProvider to avoid duplication with environment binding
What does this PR do?
#2014
What issues does this PR fix or reference?
#2014
Previous Behavior
Not possible to override configured properties with system properties or environment variables with "che." and "CHE_" prefixes correspondingly.
Not possible to have aliases for properties. Need update sources after change name of property.
See issue #2014
New Behavior
Any configuration property might be overridden with system property of environment variable "che." and "CHE_" prefixes correspondingly.
Aliases for configuration properties might be used. Do not need update source after change name of property, just add alias in file che_aliases.properties in following format:
new.name=old.name, other.old.name
Tests written?
Yes
Docs requirements?
API changes