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 attributes to Che 7 plugin meta; make URL field not mandatory #12148

Merged
merged 3 commits into from
Dec 11, 2018

Conversation

garagatyi
Copy link

What does this PR do?

Add attributes to Che 7 plugin meta; make URL field not mandatory.
This is needed for implementation of VS Code extension broker.

What issues does this PR fix or reference?

Release Notes

Docs PR

@benoitf benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/enhancement A feature request - must adhere to the feature request template. labels Dec 7, 2018
@garagatyi
Copy link
Author

ci-test

@riuvshin
Copy link
Contributor

riuvshin commented Dec 7, 2018

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12148
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

Copy link
Contributor

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

IMHO it looks strange to have check for null attributes on getter while it's initialized on class to a value. Probably check on setter or not have hashmap initialized when defining the attributes ?

@garagatyi
Copy link
Author

@benoitf we have this type of code all over the place in Che. I agree that approach suggested by you might be better. Would be better to discuss this as a separate topic with those who write server-side components and agree on what approach to use everywhere.

@garagatyi
Copy link
Author

@benoitf As far as I remember we had issues with equals on POJOs before. In a case, when one POJO instance is initialized with null or without value and another one has initialized with empty map these objects are not equal. But we never return null as a collection, so when such a POJO is used it should be an equal object.

So, we either need to ensure that collection is initialized on getter and use it in equals method or we need to initialize it with an empty map and reinitialize it on setting null as a map.
It would be better to have just one approach for such a situation because we have to use a lot of POJOs. Now we use this approach all over the place and you consider it strange. Imagine a situation when different POJOs would use different techniques - would be way more strange and error-prone.
I'm OK to change the approach in general but it is a thing that goes beyond purpose of this PR.

@garagatyi
Copy link
Author

ci-test

@riuvshin
Copy link
Contributor

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12148
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@garagatyi garagatyi merged commit 26611a5 into eclipse-che:master Dec 11, 2018
@garagatyi garagatyi deleted the vscodeBroker branch December 11, 2018 04:05
@benoitf benoitf added this to the 6.16.0 milestone Dec 11, 2018
@benoitf benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Dec 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A feature request - must adhere to the feature request template.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants