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

Added JSON-RPC back-end to Project API and removed deprecated VFS classes #6726

Closed
wants to merge 61 commits into from

Conversation

dkuleshov
Copy link

@dkuleshov dkuleshov commented Oct 13, 2017

Work is still in progress.
Original issue: #6122

Changes to Project Service:

Changes to Project Manager:

  • Refactoring and decomposition
  • Reworked package structure
  • Split Guice configuration into several classes
  • Reworked architecture (where it is possible) to comply DI pattern

Changes to VFS

  • Removed all deprecated VFS related classes
  • Rearranged code accordingly
  • Provided new FS API where needed

Changes to Search

  • Reworked index classes that depend on VFS

What is yet to be done:

  • Pass selenium tests
  • Rework/add unit tests
  • Rework/add java docs

@dkuleshov dkuleshov added kind/enhancement A feature request - must adhere to the feature request template. status/in-progress This issue has been taken by an engineer and is under active development. target/branch Indicates that a PR will be merged into a branch other than master. labels Oct 13, 2017
@dkuleshov dkuleshov self-assigned this Oct 13, 2017
@codenvy-ci
Copy link

Build # 4137 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/4137/ to view the results.

if (url.contains("?")) {
url += "&clientId=" + appContext.getApplicationId().orElse("");
} else {

Copy link
Contributor

Choose a reason for hiding this comment

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

Both parts of if should look the same.

Copy link
Author

Choose a reason for hiding this comment

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

Improved by this commit

if (url.contains("?")) {
url += "&clientId=" + appContext.getApplicationId().orElse("");
} else {

Copy link
Contributor

Choose a reason for hiding this comment

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

Both parts of if should look the same.

verify(asyncRequest).header(CONTENT_TYPE, APPLICATION_JSON);
verify(asyncRequest).send();
// client.importProject(resourcePath, source);
//
Copy link
Contributor

Choose a reason for hiding this comment

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

remove if not needed.

assertEquals(2, prjsArgCaptor.getValue().size());
// List<NewProjectConfigDto> configs = Arrays.asList(prjConfig1, prjConfig2);
//
// client.createBatchProjects(configs);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove if not needed.

Copy link
Author

Choose a reason for hiding this comment

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

This test is already reworked be one of my previous commits, in general I'm not sure if it is a good idea to review the PR that is in a work-in-progress state, I mean you will have many of your remarks outdated and useless. Please watch "status" label to be aware of current PR status not to waste your time.

Reader reader = new BufferedReader(new InputStreamReader(composerFile.getInputStream()));
return new Gson().fromJson(reader, JsonObject.class);
private JsonObject readModel(Path projectFsPath) throws ServerException, IOException {
return new Gson().fromJson(Files.newBufferedReader(projectFsPath), JsonObject.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to close reader here ?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not familiar with the logic, I did just source code adaptation to a new API, so I don't really know if we need to close reader here. If it matters for you, I guess you can ask an author.

fsManager.createDir(projectWsPath);
InputStream inputStream = getClass().getClassLoader().getResourceAsStream(RESOURCE_NAME);
String wsPath = resolve(projectWsPath, FILE_NAME);
fsManager.createFile(wsPath, inputStream);
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to close inputStream here?

Copy link
Author

Choose a reason for hiding this comment

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

Again, I'm not sure if we need to close the stream here, probably we do, however this is out of scope of this issue. The PR already has like three hundreds of file changed, honestly, I see those and some other errors, but I haven't the slightest desire to mix those fixes with this huge PR. I'm sure that there will be enough PR-related bugs and errors.

If you are really concerned about such kind of errors (and I sincerely share your concern) I guess you can create a dedicated issue so we could later get back to it and fix all those bugs.

Copy link
Author

Choose a reason for hiding this comment

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

Created an issue: #6841.

String projectWsPath, Map<String, AttributeValue> attributes, Map<String, String> options)
throws ForbiddenException, ConflictException, ServerException, NotFoundException {
fsManager.createDir(projectWsPath);
InputStream inputStream = getClass().getClassLoader().getResourceAsStream(RESOURCE_NAME);
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to close inputStream here?

//
// EventService eventService = new EventService();
// LocalVirtualFileSystemProvider vfsProvider =
// new LocalVirtualFileSystemProvider(root, sProvider);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove?

String projectCheFolderWsPath = resolve(projectWsPath, CHE_FOLDER);

if (!fsManager.existsAsDir(projectCheFolderWsPath)) {
fsManager.createDir(projectCheFolderWsPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to check if a directory has been created here?

String cheFormatterWsPath = resolve(projectCheFolderWsPath, CHE_FORMATTER_XML);

if (!fsManager.existsAsFile(cheFormatterWsPath)) {
fsManager.createFile(cheFormatterWsPath, content);
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to check if a file has been created here?

@dkuleshov
Copy link
Author

ci-test

@codenvy-ci
Copy link

ci-test build report:
Build details
Test report
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@codenvy-ci
Copy link

Build # 4185 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/4185/ to view the results.

@dkuleshov
Copy link
Author

ci-test

@codenvy-ci
Copy link

ci-test build report:
Build details
Test report
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@skabashnyuk skabashnyuk added this to the 6.0.0-M1 milestone Oct 19, 2017
Dmytro Kulieshov added 11 commits October 19, 2017 15:48
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
…classes

Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Dmytro Kulieshov added 2 commits November 8, 2017 15:45
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
@dkuleshov
Copy link
Author

ci-test

Dmytro Kulieshov added 2 commits November 8, 2017 16:23
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
@codenvy-ci
Copy link

ci-test build report:
Build details
Test report
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@dkuleshov
Copy link
Author

ci-test

@codenvy-ci
Copy link

ci-test build report:
Build details
Test report
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

Dmytro Kulieshov added 5 commits November 9, 2017 14:19
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
@dkuleshov
Copy link
Author

ci-test

@codenvy-ci
Copy link

ci-test build report:
Build details
Test report
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

tolusha and others added 2 commits November 10, 2017 15:00
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
@dkuleshov
Copy link
Author

ci-test

Dmytro Kulieshov added 3 commits November 13, 2017 15:57
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
Signed-off-by: Dmytro Kulieshov <dkuliesh@redhat.com>
@dkuleshov
Copy link
Author

ci-test

@codenvy-ci
Copy link

ci-test build report:
Build details
Test report
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@benoitf
Copy link
Contributor

benoitf commented Nov 15, 2017

it is normal this PR has been closed ? looks like it was a work in progress since one month ?

@dkuleshov
Copy link
Author

Created a new PR instead of this one #7380

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. status/in-progress This issue has been taken by an engineer and is under active development. target/branch Indicates that a PR will be merged into a branch other than master.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants