-
Notifications
You must be signed in to change notification settings - Fork 111
chore(plugins-view): plugin list was not displayed for DevWorkspace instances #1225
Conversation
…nstances Signed-off-by: Vitaliy Gulyy <[email protected]>
✅ E2E Happy path tests succeed 🎉 See Details
Test product:
Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe |
console.log('workspaceService.getWorkspaceSettings() is implemented partially'); | ||
|
||
return { | ||
CHE_PLUGIN_REGISTRY_URL: this.env.getPluginRegistryURL(), |
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.
note that these variables are available in Che/DevWorkspaceOperator mode but not in pure DevWorkspace Operator mode so please check that there is no error on pure DevWorkspace
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.
then, how can we get links to plugin registry in pure DevWorkspace Operator mode?
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.
it requires ad-hoc feature but usually everything is provided by config map
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.
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.
Pure DevWorkspace Operator mode means, that there is no plugin registry, right?
Then how Che-Theia should work in that case? Is it OK to display the same notification ( Your registry is invalid ) as we have now?
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.
we could display the notification or use online instance of the registry at github.io
Signed-off-by: Vitaliy Gulyy <[email protected]>
…nstances Signed-off-by: Vitaliy Gulyy <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #1225 +/- ##
==========================================
- Coverage 32.78% 32.31% -0.47%
==========================================
Files 290 296 +6
Lines 9885 9849 -36
Branches 1457 1322 -135
==========================================
- Hits 3241 3183 -58
- Misses 6641 6662 +21
- Partials 3 4 +1
Continue to review full report at Codecov.
|
❌ E2E Happy path tests failed ❗ See Details
Test product:
Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe |
Signed-off-by: Vitaliy Gulyy <[email protected]>
❌ E2E Happy path tests failed ❗ See Details
Test product:
Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe |
…nstances Signed-off-by: Vitaliy Gulyy <[email protected]>
❌ E2E Happy path tests failed ❗ See Details
Test product:
Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe |
extensions/eclipse-che-theia-remote-impl-k8s/src/node/k8s-devworkspace-env-variables.ts
Outdated
Show resolved
Hide resolved
if (publicURI && privateURI) { | ||
return { | ||
CHE_PLUGIN_REGISTRY_URL: publicURI, | ||
CHE_PLUGIN_REGISTRY_INTERNAL_URL: privateURI, |
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.
If we call these keys without CHE_
prefix, as PLUGIN_REGISTRY_URL
and PLUGIN_REGISTRY_INTERNAL_URL
, then we don't need any changes in che-plugin-service.ts
. Isn't it?
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.
PLUGIN_REGISTRY_URL in che-plugin-service.ts
means a bit different
const PLUGIN_REGISTRY_URL = 'cheWorkspacePluginRegistryUrl';
I would use CHE_PLUGIN_REGISTRY_URL
for both cases and remove cheWorkspacePluginRegistryUrl
, but the value is given from che-server.
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.
PLUGIN_REGISTRY_URL in che-plugin-service.ts means a bit different
Actually, it means the same - URL of the plugin registry. It doesn't matter how this registry is implemented and where it's located, etc.
workspaceService.getWorkspaceSettings()
allows to abstract from where it's stored: either Che-Server WS settings or DevWorkspace env. variables/config map.
So, if we get rid of the CHE_
prefixes, we can make it more transparent in che-plugin-service.ts
.
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.
PLUGIN_REGISTRY_URL
it's only name of the variable, but yes, it means the same - URI to the plugin registry.
const PLUGIN_REGISTRY_URL = 'cheWorkspacePluginRegistryUrl';
But, if you prefer to have less changes, which is reasonable, we can return the object with allready-known keys
return {
cheWorkspacePluginRegistryUrl: publicURI,
cheWorkspacePluginRegistryUrl: privateURI,
};
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.
looks like the returned object has the same key but with different values ?
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.
Changes in che-plugin-service.ts
were reverted, k8s-workspace-service-impl.ts
returns
return {
cheWorkspacePluginRegistryUrl: publicURI,
cheWorkspacePluginRegistryUrl: privateURI,
};
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.
looks like the returned object has the same key but with different values ?
I would say different keys with the same meaning.
console.log('workspaceService.getWorkspaceSettings() not supported'); | ||
return {}; | ||
const publicURI = this.env.getPluginRegistryURL(); | ||
const privateURI = this.env.getPluginRegistryInternalURL() || publicURI; |
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.
Is there a real reason for using a public URI as a private one if it is not set?
Considering we're already checking that in
che-theia/extensions/eclipse-che-theia-plugin-ext/src/node/che-plugin-service.ts
Line 103 in f677d11
const uri = workspaceSettings[PLUGIN_REGISTRY_INTERNAL_URL] || publicUri; |
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.
We use private URI for cross-container communication, to allow che-theia get the plugin list from apache server, that is running in che-plugin-registry.
When internal URI is not defined, we use public URI. That for cases when user defined his plugin registry outside Che and added it to che-theia, using only one URI.
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.
Thanks for the explanation!
Do we need to check it twice? In both k8s-workspace-service-impl.ts
and che-plugin-service.ts
.
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.
Yes, we still need both of them.
Private URI is used to get the plugin list on the backend, public URI is used by frontend to load plugin icons.
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'm not asking about both URIs.
I'm asking about both places of using it:
che-theia/extensions/eclipse-che-theia-remote-impl-k8s/src/node/k8s-workspace-service-impl.ts
Line 75 in 4f6995d
const privateURI = this.env.getPluginRegistryInternalURL() || publicURI; che-theia/extensions/eclipse-che-theia-plugin-ext/src/node/che-plugin-service.ts
Line 103 in f677d11
const uri = workspaceSettings[PLUGIN_REGISTRY_INTERNAL_URL] || publicUri;
In other words, if I want to get and see the "raw" settings returned by workspaceService.getWorkspaceSettings()
, I should be able to get it as it is. Without any substitutions inside, like in 1.
And only when I get the "raw" settings, I can do with them whatever I want - like making such checks as in 2.
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.
Private URI is used to get the plugin list on the backend, public URI is used by frontend to load plugin icons
workspaceService.getWorkspaceSettings()
knows nothing about that ^. And it must not know that.
It's only ChePluginService
who has this knowledge about plug-ins, icons and so on.
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.
Let's imagine, in some other place in the code, I call workspaceService.getWorkspaceSettings()
in order to see if privateURI
is absent. Despite the presence of publicURI
.
How I can know that? There's no chance.
That's why I'm proposing to return the WorkspaceSettings
as it is. And keep such checking only in the place where it's actually important - like in ChePluginService
.
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.
Let's imagine, in some other place in the code, I call
workspaceService.getWorkspaceSettings()
in order to see ifprivateURI
is absent. Despite the presence ofpublicURI
. How I can know that? There's no chance. That's why I'm proposing to return theWorkspaceSettings
as it is. And keep such checking only in the place where it's actually important - like inChePluginService
.
Got it, reworking.
…nstances Signed-off-by: Vitaliy Gulyy <[email protected]>
❌ E2E Happy path tests failed ❗ See Details
Test product:
Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe |
Signed-off-by: Vitaliy Gulyy <[email protected]>
✅ E2E Happy path tests succeed 🎉 See Details
Test product:
Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe |
Signed-off-by: Vitaliy Gulyy [email protected]
What does this PR do?
Implements
K8sWorkspaceServiceImpl.getWorkspaceSettings()
method to be able Che-Theia get plugin registry internal/external URLs.Screenshot/screencast of this PR
What issues does this PR fix or reference?
eclipse-che/che#20448
How to test this PR?
Devfile to create a workspace
To create a workspace use this public Gist https://gist.githubusercontent.com/vitaliy-guliy/c3eadb47d3c241d9a6632029e580bf91/raw/802bab8a45fd388c3e91a26cc98a1f083adaf2dc
PR Checklist
As the author of this Pull Request I made sure that:
What issues does this PR fix or reference
andHow to test this PR
completedReviewers
Reviewers, please comment how you tested the PR when approving it.
Happy Path Channel
HAPPY_PATH_CHANNEL=stable