-
Notifications
You must be signed in to change notification settings - Fork 150
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 Camel WSDl2Rest VS Code Extension (#120) - revision #136
Add Camel WSDl2Rest VS Code Extension (#120) - revision #136
Conversation
acaff40
to
2361847
Compare
Attempted to figure out what to do with the v3 changes to the yaml file, but have no idea how to fill out the spec portion. |
Plus we still have the eclipse-che/che#13232 issue to resolve. |
@bfitzpat we will move to v3 quite soon. Could you also add this plugin to v3 folder (with adaptation to meta.yaml structure that v3 uses). You can take a look at other plugin to see how meta.yaml looks in v3 |
If you don't add your plugin to v3 this won't break this PR actually. You would be able to successfully merge it. But when we move Che to usage of registry v3 plugin will be absent. |
I will see about getting this done today. Thanks |
2361847
to
8ad0eb8
Compare
@garagatyi - Looks like I have added the v3 bits correctly. |
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 good!
8ad0eb8
to
9e04669
Compare
Now that eclipse-che/che#13232 is no longer an issue, can we merge this? I tested it on a che.openshift.io instance and it worked great today. |
9e04669
to
56d2a15
Compare
56d2a15
to
8d6570a
Compare
Updated PR to point to latest vsix build at new location for Red Hat published artifacts |
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.
- folders needs to be updated from camel-tooling to redhat
- extension ids needs to be updated from camel-tooling.vscode-wsdl2rest to redhat.vscode-wsdl2rest
8d6570a
to
5d48720
Compare
Fixed @apupier |
for information, ahead of time. This PR will conflict with this one #196 |
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.
As others have mentioned, the v1
and v2
plugin metas should maybe not be included in this PR, as I believe only beta and rc releases of Che use these paths and they will be removed in the (near) future.
spec: | ||
containers: | ||
- image: "eclipse/che-remote-plugin-runner-java8:next" | ||
memoryLimit: "1000Mi" |
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 this a minimum requirement for memory? With a requirement of 1000Mi
, running a workspace with this plugin and the Java LS will already consume over 3Gi
.
From an OSIO perspective, it would be great if this default limit could be lowered.
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.
Other than thru trial and error is there a way for us to determine how much memory is actually needed @amisevsk ? Or do we even need that container? I was just going by what was being done by other extensions
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.
@bfitzpat trial and error is thus far the best approach I've found; I usually open a terminal and monitor memory usage while trying to tax the plugin (e.g. for Java LS, view mem usage while its importing the project). From there I adjust up a little.
As for whether the container is needed, I would ask one of our theia/plugin devs. I can't confidently say one way or another. Some plugins don't require sidecars, e.g. the github pull request plugin.
8976cae
to
b63e68d
Compare
@amisevsk I have removed the v1 and v2 versions of the metadata for this extension/plugin. And I tested in che.openshift.io with the smaller memory footprint (256Mi) -- seems to work fine. Please let me know if you need anything else |
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, thanks. One concern is that including this plugin in the current apache-camel devfile will be difficult due to memory quotas on osio, but that's a separate issue.
yes |
I plan to have a look at it at the beginning of September. |
I wasn't able to manage to change the registry locally but I was able to point to the files directly from a workspace configuration to test it on a 7.1.0-SNAPSHOT Che version
it works fine. |
can one of the Code Owner review it too so that I can merge it? @l0rd @rhopp @skabashnyuk @vparfonov |
Signed-off-by: Brian Fitzpatrick <[email protected]>
…gistry Signed-off-by: Brian Fitzpatrick <[email protected]>
…: Brian Fitzpatrick <[email protected]> Signed-off-by: Brian Fitzpatrick <[email protected]>
Signed-off-by: Brian Fitzpatrick <[email protected]>
… names, adusting yaml, adding 0.0.9, adjusting vsix location -removing v1 and v2 versions -adjusting memory Signed-off-by: Brian Fitzpatrick <[email protected]>
b63e68d
to
2d793d0
Compare
Signed-off-by: Brian Fitzpatrick [email protected]
create Pull request to check if tests are passing.
What does this PR do?
What doesn't this PR do?