-
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
Add internal and external API URL environment variables for workspaces #9475
Conversation
import org.eclipse.che.api.workspace.server.spi.InfrastructureException; | ||
import org.eclipse.che.commons.lang.Pair; | ||
|
||
/** @author Sergii Leshchenko */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix authorship here
|
||
@Inject | ||
public DockerCheApiExternalEnvVarProvider() { | ||
apiEnvVar = Pair.of(CHE_API_EXTERNAL_VARIABLE, System.getenv(CHE_API_EXTERNAL_VARIABLE)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain why do you use System.getenv(CHE_API_EXTERNAL_VARIABLE)
but not injected che.api
constant
@Inject | ||
public DockerCheApiInternalEnvVarProvider( | ||
@Named("che.infra.docker.master_api_endpoint") String apiEndpoint) { | ||
String apiEndpointEnvVar = System.getenv(CHE_API_INTERNAL_VARIABLE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say that it should be removed. If a user wants to customize this value he is able to specify CHE_INFRA_DOCKER_MASTER__API__ENDPOINT
environment variable.
…se/che into che-9323
…m:eclipse/che into che-9323
ci-test |
ci-test build report: |
…thub.com:eclipse/che into che-9323
ci-test |
ci-test build report: |
@@ -51,7 +51,6 @@ CHE_LOGS_DIR=/logs | |||
CHE_WORKSPACE_LOGS=/logs/machines | |||
CHE_TEMPLATE_STORAGE=/data/templates | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this change if not needed
dockerfiles/init/manifests/che.env
Outdated
@@ -160,7 +160,7 @@ | |||
# | |||
# Browser --> Che Server | |||
# 1. Default is 'http://localhost:${SERVER_PORT}/api'. | |||
# 2. Else use the value of CHE_API | |||
# 2. Else use the value of CHE_API_EXTERNAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we need to remove this change too.
@@ -37,6 +37,7 @@ public void provision(RuntimeIdentity id, InternalEnvironment internalEnvironmen | |||
throws InfrastructureException { | |||
for (EnvVarProvider envVarProvider : envVarProviders) { | |||
Pair<String, String> envVar = envVarProvider.get(id); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this change if not needed
ci-test |
ci-test build report: |
ci-test |
ci-test build report: |
ci-test |
ci-test build report: |
ci-test |
ci-test |
ci-test build report: |
String CHE_API_INTERNAL_VARIABLE = "CHE_API_INTERNAL"; | ||
|
||
/** | ||
* Returns Che API environment variable which should be injected into machines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document what is internal Che API URL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -67,6 +71,8 @@ protected void configure() { | |||
bind(RemoveProjectOnWorkspaceRemove.class).asEagerSingleton(); | |||
|
|||
bind(CheApiEnvVarProvider.class).to(KubernetesCheApiEnvVarProvider.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need CHE_API
env var?
Or it wasn't removed because of backward compatibility? If yes, then maybe it would be better to ensure it on Workspace API level instead of each infrastructure?
I mean creating CheApiEnvVarProvider
class that will inject CheApiInternalEnvVarProvider
and provide its value as CHE_API
. WDYT?
ci-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
||
/** | ||
* Returns Che API environment variable which should be injected into machines. | ||
* Internal API URL is meant to be used from the inside of other machine containers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't use containers
word here. I think other machines
would be fine
ci-test build report: |
What does this PR do?
All workspaces will be provided with additional environment variables, that will point to internal or external API URL. On Docker infrastructure:
In Openshift and Kubernetes,CHE_API, CHE_API_INTERNAL, CHE_API_EXTERNAL will be the same and based from "che.api" property.
What issues does this PR fix or reference?
#9323
Release Notes
Docs PR