Skip to content
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

Merged

Conversation

bfitzpat
Copy link
Contributor

@bfitzpat bfitzpat commented May 2, 2019

Signed-off-by: Brian Fitzpatrick bfitzpat@redhat.com

create Pull request to check if tests are passing.

What does this PR do?

  • this PR is adding the Camel WSDL2Rest extension to the list of extensions in Che
  • also adds new plugin to the v2 folder
  • also updates meta.yaml formatting for v2

What doesn't this PR do?

  • add v3 support. I attempted to figure out what needed to be added to the v3 yaml and got lost quickly.

@bfitzpat bfitzpat force-pushed the 120-AddVSCodeWSDl2RestVSCodeExtension branch 7 times, most recently from acaff40 to 2361847 Compare May 2, 2019 17:08
@bfitzpat
Copy link
Contributor Author

bfitzpat commented May 2, 2019

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.

@bfitzpat
Copy link
Contributor Author

bfitzpat commented May 2, 2019

Plus we still have the eclipse-che/che#13232 issue to resolve.

@garagatyi
Copy link

@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

@garagatyi
Copy link

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.

@bfitzpat
Copy link
Contributor Author

bfitzpat commented May 3, 2019

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

@bfitzpat bfitzpat force-pushed the 120-AddVSCodeWSDl2RestVSCodeExtension branch from 2361847 to 8ad0eb8 Compare May 3, 2019 13:10
@bfitzpat
Copy link
Contributor Author

bfitzpat commented May 3, 2019

@garagatyi - Looks like I have added the v3 bits correctly.

Copy link

@garagatyi garagatyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@bfitzpat bfitzpat force-pushed the 120-AddVSCodeWSDl2RestVSCodeExtension branch from 8ad0eb8 to 9e04669 Compare May 29, 2019 17:21
@bfitzpat
Copy link
Contributor Author

bfitzpat commented Jul 2, 2019

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.

@bfitzpat
Copy link
Contributor Author

Updated PR to point to latest vsix build at new location for Red Hat published artifacts

Copy link
Contributor

@apupier apupier left a 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

@bfitzpat bfitzpat force-pushed the 120-AddVSCodeWSDl2RestVSCodeExtension branch from 8d6570a to 5d48720 Compare July 17, 2019 15:21
@bfitzpat
Copy link
Contributor Author

* 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

Fixed @apupier

@l0rd
Copy link
Contributor

l0rd commented Jul 17, 2019

@apupier @bfitzpat we are on stabilisation period and merge only PRs that fix some endgame issue.

@bfitzpat
Copy link
Contributor Author

@apupier @bfitzpat we are on stabilisation period and merge only PRs that fix some endgame issue.

Yes, we're just getting ready for when [endgame] is done and things open up again. Thanks!

@apupier
Copy link
Contributor

apupier commented Jul 30, 2019

for information, ahead of time. This PR will conflict with this one #196

Copy link
Contributor

@amisevsk amisevsk left a 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"
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

@bfitzpat bfitzpat force-pushed the 120-AddVSCodeWSDl2RestVSCodeExtension branch 3 times, most recently from 8976cae to b63e68d Compare July 30, 2019 21:26
@bfitzpat
Copy link
Contributor Author

@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

Copy link
Contributor

@amisevsk amisevsk left a 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.

@amisevsk
Copy link
Contributor

@l0rd Should this PR be merged for 7.0.0?

@apupier I believe the changes you requested have been implemented.

@apupier
Copy link
Contributor

apupier commented Aug 1, 2019

@apupier I believe the changes you requested have been implemented.

yes

@l0rd
Copy link
Contributor

l0rd commented Aug 22, 2019

@apupier @amisevsk che 7.0.0 has been released and we can proceed with the merge of this PR if you are ready

@apupier
Copy link
Contributor

apupier commented Aug 22, 2019

I plan to have a look at it at the beginning of September.

@apupier apupier self-assigned this Sep 4, 2019
@apupier
Copy link
Contributor

apupier commented Sep 5, 2019

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

components:
  - id: >-
      https://raw.githubusercontent.com/bfitzpat/che-plugin-registry/120-AddVSCodeWSDl2RestVSCodeExtension/v3/plugins/redhat/vscode-wsdl2rest/latest/meta.yaml
    memoryLimit: 128Mi
    type: chePlugin

it works fine.

@apupier
Copy link
Contributor

apupier commented Sep 5, 2019

can one of the Code Owner review it too so that I can merge it? @l0rd @rhopp @skabashnyuk @vparfonov

Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>
…gistry

Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>
…: Brian Fitzpatrick <bfitzpat@redhat.com>

Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>
Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>
… names, adusting yaml, adding 0.0.9, adjusting vsix location -removing v1 and v2 versions -adjusting memory

Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>
@apupier apupier force-pushed the 120-AddVSCodeWSDl2RestVSCodeExtension branch from b63e68d to 2d793d0 Compare September 9, 2019 11:57
@apupier apupier assigned l0rd, rhopp, skabashnyuk and vparfonov and unassigned apupier Sep 9, 2019
@apupier apupier merged commit 57fa808 into eclipse-che:master Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants