-
Notifications
You must be signed in to change notification settings - Fork 60
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 support for plugins referenced by URI or in external registries #269
Conversation
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.
Everything makes sense to me, the only problem I run into when testing is:
reconnecting-websocket-mjs.js:518 WebSocket connection to 'wss://workspace8ea81d13d3e24820.apps-crc.testing/theia/services' failed: Error during WebSocket handshake: Unexpected response code: 404
(anonymous) @ reconnecting-websocket-mjs.js:518
so I'm not actually able to get into theia and test that everything is working correctly, the loader just keeps spinning
pkg/library/flatten/flatten.go
Outdated
plugin *devworkspace.PluginComponent, | ||
tools ResolverTools) (resolvedPlugin *devworkspace.DevWorkspaceTemplateSpec, pluginLabels map[string]string, err error) { | ||
|
||
if plugin.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.
Can this piece be hit? It looks like on line ~135 ish you have case plugin.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.
You're right, it can't -- I suppose it's moreso paranoia on my part from constantly having to check component.Plugin != nil
😄
I'll remove the check (this also increases our test coverage 😉 !)
) | ||
|
||
const ( | ||
SupportedDevfileSchemaVersion = "2.0.0" // TODO what should this actually be at this point? |
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.
// TODO what should this actually be at this point?
I'd like to know the answer on that as well, I've just been using 2.0.0
on the dashboard side 😆
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.
2.0.0 don't support setting cpuLimits, while 2.1.0 supports )
Probably we need support all and convert to the latest.
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.
Realistically, it should be something like 2\..*\..*
, assuming there's no backwards-incompatible changes.
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.
Replaced this with a basic ^2\..*
regexp.
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 followed the instructions for OpenShift and my workspace is RUNNING but I'm not able to access Che Theia due 404 -> Cannot GET /theia/
I have RHPDS cluster, but this issue is related more to Che Theia or single host per workspace, which I implemented and merged long time ago. So, I don't see how it's related to the current PR.
The changes LGTM, my testing RHPDS as almost successful, plugins are fetched and workspace pod is correctly created
pkg/library/flatten/network/fetch.go
Outdated
Get(location string) (*http.Response, error) | ||
} | ||
|
||
func FetchDevWorkspaceTemplate(location string, httpClient HTTPGetter) (*dw.DevWorkspaceTemplateSpec, map[string]string, error) { |
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.
this method does not have doc and even return params are not named. At least it's not clear what map is supposed to contain.
Consider fixing one of them or both
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.
Removed any handling of plugin metadata until we decide a plan for how to do it.
) | ||
|
||
const ( | ||
SupportedDevfileSchemaVersion = "2.0.0" // TODO what should this actually be at this point? |
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.
2.0.0 don't support setting cpuLimits, while 2.1.0 supports )
Probably we need support all and convert to the latest.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amisevsk, JPinkney, sleshchenko The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
New changes are detected. LGTM label has been removed. |
/test v5-devworkspaces-operator-e2e |
1 similar comment
/test v5-devworkspaces-operator-e2e |
e244837
to
98bc81a
Compare
Signed-off-by: Angel Misevski <[email protected]>
Signed-off-by: Angel Misevski <[email protected]>
Signed-off-by: Angel Misevski <[email protected]>
Signed-off-by: Angel Misevski <[email protected]>
Signed-off-by: Angel Misevski <[email protected]>
Signed-off-by: Angel Misevski <[email protected]>
Avoid processing plugin metadata as there's still not defined way to translate metadata from a devfile into a DevWorkspace[Template] Signed-off-by: Angel Misevski <[email protected]>
98bc81a
to
69aa54c
Compare
/test v5-devworkspaces-operator-e2e Had to rebase over #270 and update imports to include |
What does this PR do?
Adds support for downloading plugins via URI, enabling support for
components.plugin.uri
andcomponents.plugin.id
whenregistryUrl
is specified. Adds tests to cover new functions.What issues does this PR fix or reference?
Closes #266
Is it tested? How?
The easiest way to test this PR is probably to deploy the plugin registry from this repo:
Apply a devworkspace referring to this registry: