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

CHE-745 Make plugin-svn build & run in Che #758

Merged
merged 1 commit into from
Mar 30, 2016
Merged

Conversation

stour
Copy link

@stour stour commented Mar 21, 2016

This Pull Request:

  • Refactor plugin-svn to make it build and run in Che 4.x
  • Adds plugin-svn in che/plugins but not as part of default Che assembly as some further refactoring are needed in order to have all commands working properly.
    See epic for details.

@skabashnyuk
Copy link
Contributor

Will we have svn support in Che enabled?

@TylerJewell
Copy link

Seems like a good idea.

}

private void refreshNodes(String forPath) {
// appContext.getCurrentProject().getCurrentTree().getNodeByPath(forPath, new AsyncCallback<TreeNode<?>>() {
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 this code?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, this code is outdated to the new codebase in 4.x and need to be rewritten

Copy link
Contributor

Choose a reason for hiding this comment

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

Deprecated code should be removed.

@skabashnyuk
Copy link
Contributor

Can you confirm legal origin of all source code and images?
I have a feeling that we might need CQ to ensure legal clarity of this code

@skabashnyuk
Copy link
Contributor

Especially due to use of crypto algorithms

*******************************************************************************/
package org.eclipse.che.api.crypt.server;

public interface EncryptTextServiceRegistry {
Copy link
Contributor

Choose a reason for hiding this comment

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

javadoc

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 adding the javadoc.

About the "legal origin" I guess that you want to be sure that the whole code was written by us and does not contain some copy/paste code. I asked Mickael (who created this encryption code) and he confirm that the whole code is from him. Not copy/paste. So no legal issue.
About copyright on images they were already double checked when we added them originally.

@stour
Copy link
Author

stour commented Mar 21, 2016

95% of this code is the same as the original SVN plugin that we had in Che 3. The other 5% is what I refactored to make the plugin compliant with Che 4.x. And I added no image or "critical" code.

/**
* The encryption services.
*/
private final Map<Integer, EncryptTextService> registry = new HashMap<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

better no create in constructor with known size

@stour stour force-pushed the CHE-745 branch 4 times, most recently from e5b0d6f to 8d12599 Compare March 25, 2016 14:58
@stour
Copy link
Author

stour commented Mar 25, 2016

@skabashnyuk @sleshchenko I applied changes related to your comments + other needed changes like creating separate submodules for server, ide and shared parts.

@skabashnyuk
Copy link
Contributor

Ok. to go after 4.0


/**
* Extension adding <a href="http://subversion.apache.org">Subversion</a> support to the
* <a href="http://codenvy.com">Codenvy</a> IDE.
Copy link
Member

Choose a reason for hiding this comment

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

Rewrite comment without Codenvy

* Interface providing access to localization constants.
* <p/>
* These are loaded from
* src/resources/org/eclipse/che/plugin/svn/SubversionExtensionLocalizationConstants.properties
Copy link
Member

Choose a reason for hiding this comment

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

Remove or rewrite this outdated comment. For now it locates in package org/eclipse/che/plugin/svn/ide
I prefer to remove it.

@stour stour force-pushed the CHE-745 branch 4 times, most recently from 44901d5 to 251e1a8 Compare March 30, 2016 08:39
Signed-off-by: Stéphane Tournié <stephane.tournie@serli.com>
@vparfonov
Copy link
Contributor

ok

@stour stour merged commit d8fe03b into eclipse-che:master Mar 30, 2016
@stour stour deleted the CHE-745 branch March 30, 2016 09:05
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.

6 participants