-
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
Adjust Workspace.Next model classes to latest updates in the Workspace.Next vision #10246
Conversation
Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
ci-test |
ci-test build report: |
Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
@sleshchenko @l0rd @ibuziuk @skabashnyuk Please, review the PR |
assertEquals(memoryLimitAttribute, Integer.toString(MEMORY_LIMIT_MB * 1024 * 1024)); | ||
} | ||
|
||
private ChePlugin testChePlugin() { |
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.
In my opinion, testChePlugin
sounds like it should test ChePlugin, but in facts, it creates test ChePlugin object. It does not make much sense since it is test sources but maybe just createChePlugin
would be clearer.
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.
ok, will change 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.
done
Map<String, List<String>> parameters = new HashMap<>(); | ||
for (CheFeature feature : features) { | ||
for (CheServiceReference serviceReference : feature.getSpec().getServices()) { | ||
if (serviceReference.getParameters().isEmpty()) { | ||
for (ChePluginReference pluginReference : feature.getSpec().getServices()) { |
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.
Should be feature.getSpec().getServices()
method renamed to feature.getSpec().getFeature()?
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 going to update Feature related model as we are going to remove this entity from the model according to the discussion in workspace next walking skeleton #10123
@@ -70,7 +70,7 @@ public boolean equals(java.lang.Object o) { | |||
if (o == null || getClass() != o.getClass()) { | |||
return false; | |||
} | |||
CheServiceReference cheServiceReference = (CheServiceReference) o; | |||
ChePluginReference cheServiceReference = (ChePluginReference) o; |
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.
cheServiceReference -> chePluginReference
Please make sure that all usages of service
is replaced with plugin
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 going to update Feature related model as we are going to remove this entity from the model according to the discussion in workspace next walking skeleton #10123
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.
OK, but it's not about feature model changing, but about variable name =)
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
Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
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. I have 2 comments that we can discuss and address in separate issues:
- you are including the switch to apache for the marketplace (that was not part of the model adujstment) but I think it should live in a separate github repository
- you are basing everything on the
kubectl
deployment type instead of usinghelm
whereas I think helm should be the default
I wanted to test the PR on minishift by deploying che-server with feature-api in the same namespace and creating workspace via:
Workspace is started correctly but for some reason I do not have Theia server with URL to IDE:
Any ideas what I'm doing wrong ? |
@l0rd I switched to Apache because didn't want to do model changes for kubsrv service in Golang. I think it is easier to support for the time being. Can you elaborate what exactly you would want to move to another repository? |
@ibuziuk I answered in MM channel |
@garagatyi I would move the content of folder |
…e.Next vision (eclipse-che#10246) Rework model, of CheService and renamed it to ChePlugin. Replace features hosting to apache server to be able to host plugin files and avoiding having model files for Go lang. Host YAMLs instead of JSONs. Downloads and parse ChePlugin YAMLs instead of JSONs from the marketplace. Improve unit tests coverage. Add support of Che Server protocol and path, so it is possible to run Classic GWT IDE in Workspace Next now. Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
What does this PR do?
This PR rework Workspace.Next model classes in accordance with #9964 .
Also, it replaces kubsrv service with Apache web server that hosts Workspace.Next Feature/ChePlugin files. They are stored in YAML format instead of JSON (comparing to kubsrv).
Also added support of Che Server protocol and path, so it is possible to run Classic GWT IDE in Workspace Next now.
PR contains a dockerfile for building marketplace placeholder service and it is possible to replace content with other files, so developers can play with it during development.
Additional unit tests were also added.
What issues does this PR fix or reference?
#9964
Release Notes
Docs PR