CODENVY-1921: BitbucketServer adaptations #2000

vinokurig · 2017-03-28T15:07:14Z

What does this PR do?

Fixed method that gets authorized Bitbucket Server user and fixed the Bitbucket Pull Request plugin to be able to make pull requests from forks if user != repo owner, otherwise from branch.
Implemented 'update Pull Request' logic in Bitbucket and Bitbucket Server plugins.
Fixed bug when successful update pull request step was not marked as successful in UI.
Rework Bitbucket Server Integration plugin to be able to work with repositories created from project.
Moved validate bitbucket url logic to the step of system properties initialization.
Divided Bitbucket Server plugin to 3 maven modules: client, server, shared.

What issues does this PR fix or reference?

#1921

Changelog

Allow PR panel and integration plugins to use BitBucket repos created from projects not just users.
Added logic in PR panel to pull from fork if user is not repo owner, otherwise from branch.
Fixed bug that prevented successful pull request from being marked correctly in UI.
Moved validate bitbucket url logic to system properties initialization step.
Divided Bitbucket Server plugin to 3 maven modules: client, server, shared.

Release Notes

Enhanced BitBucket Integration Support
We have made enhancements to our BitBucket support to allow the JIRA and Jenkins integrations to use project-based repos in addition to the personal repos already supported. We have also added logic to the pull request panel inside Codenvy to issue the pull request from a fork if the user executing the pull request is not the repo owner. If the user is a repo owner the PR will be issued from the branch. Finally we have fixed a bug which resulted in a successfully executed PR from being properly marked as successful in the pull request panel UI.

Docs PR

Bugfix

codenvy/docs#103

bmicklea · 2017-03-28T15:55:49Z

@skabashnyuk and @sleshchenko - can you review this? Fidelity needs this in 5.6.

bmicklea

updated issue description with changelog, release notes and docs link.

sleshchenko · 2017-03-29T07:08:46Z

plugins/plugin-bitbucket/codenvy-plugin-bitbucket-ext-bitbucket-client/pom.xml

+        </dependency>
+    </dependencies>
+    <build>
+        <sourceDirectory>src/main/java</sourceDirectory>


I think following lines are redundant

+ <sourceDirectory>src/main/java</sourceDirectory> + <testSourceDirectory>src/test/java</testSourceDirectory> + <outputDirectory>target/classes</outputDirectory>

sleshchenko · 2017-03-29T07:09:47Z

...et-client/src/main/java/org/eclipse/che/ide/ext/bitbucket/client/BitbucketClientService.java

@@ -87,11 +85,9 @@ private String getBaseUrl() {
    /**
     * Returns the promise which resolves authorized user information or rejects with an error.
     *


sleshchenko · 2017-03-29T07:11:12Z

plugins/plugin-bitbucket/codenvy-plugin-bitbucket-ext-bitbucket-server/pom.xml

-            <groupId>org.mockito</groupId>
-            <artifactId>mockito-core</artifactId>
-            <scope>test</scope>
-        </dependency>
    </dependencies>
    <build>
        <sourceDirectory>src/main/java</sourceDirectory>


I think you can remove sourceDirectory and outputDirectory along testSourceDirectory because they have default values

sleshchenko · 2017-03-29T08:43:56Z

plugins/plugin-bitbucket/codenvy-plugin-bitbucket-ext-bitbucket-client/pom.xml

+        </dependency>
+        <dependency>
+            <groupId>com.google.inject</groupId>
+            <artifactId>guice</artifactId>


We use gin for gwt code. Why do we need guice dependency here? I suppose it's because usage of com.google.inject.Inject. Can we replace them with javax.inject.Inject and remove this dependency?

com.google.inject.Singleton is used in generated DtoClientImpls

😞 OK. thanks for explanation

sleshchenko · 2017-03-29T09:50:01Z

...ucket-server/src/main/java/org/eclipse/che/ide/ext/bitbucket/server/BitbucketConnection.java

+     * @param owner
+     *         the repository owner
+     * @param repositorySlug
+     *         the repository name


As far as I understand repository name and repository slug are different things https://github.com/codenvy/codenvy/pull/2000/files#diff-66c3fbda6d05a3029ff6c6e25921e59cR27

changed description to url compatible repository name

sleshchenko · 2017-03-29T09:53:29Z

...pullrequest/src/main/java/com/codenvy/plugin/pullrequest/client/BitbucketHostingService.java

@@ -146,7 +151,9 @@ public boolean isHostRemoteUrl(final String remoteUrl) {
                                                 final BitbucketPullRequestLocation source = pullRequest.getSource();
                                                 if (author != null && source != null) {
                                                     final BitbucketPullRequestBranch branch = source.getBranch();
-                                                     if (username.equals(author.getUsername()) && branchName.equals(branch.getName())) {
+                                                     //Bitbucket Server adds '~' to authenticated user, need to substring it.
+                                                     String userName = username.startsWith("~") ? username.substring(1) : username;


It's hard to say that it is good practice to have userName and username variables

used name

sleshchenko · 2017-03-29T10:03:30Z

...ket-server/src/main/java/org/eclipse/che/ide/ext/bitbucket/server/BitbucketURLTemplates.java

@@ -41,6 +41,11 @@ public String pullrequestUrl(String owner, String repositorySlug) {
    }

    @Override
+    public String updatePullrequestUrl(String owner, String repositorySlug, int pullrequestId) {


Why we can't write updatePullrequestUrl in camel case here? I mean updatePullRequestUrl

sleshchenko · 2017-03-29T10:07:44Z

...ket-server/src/main/java/org/eclipse/che/ide/ext/bitbucket/server/rest/BitbucketService.java

+                                                                                           ServerException {
+        return bitbucketConnection.updatePullRequest(owner, repositorySlug, pullRequest);
+    }
+


Please take a look https://github.com/codenvy/codenvy/pull/2000/files#diff-5e9c059cfdf4fce8aaf00131f0c424adR155 As far as I remember we've started use vcs instead of git for service name. It can be a problem with showing such keys by IDE. Please check that IDE shows new generated keys.

As far as I understand this https://github.com/codenvy/codenvy/pull/2000/files#diff-5e9c059cfdf4fce8aaf00131f0c424adR152 won't work for bitbucket server that can have different host. Am I correct?

Changed to vcs.

This API method is not used in Bitbucket Server implementation.

You did it in one place, but there is few more. Please do the same there. Full text search can help you 😉

Thanks, changed :)

…VY-1921

skabashnyuk

ok for me.final go up to @sleshchenko

sleshchenko · 2017-03-29T13:06:28Z

plugins/plugin-bitbucket/codenvy-plugin-bitbucket-ext-bitbucket-client/pom.xml

+                <directory>${project.build.directory}/generated-sources/dto/</directory>
+            </resource>
+        </resources>
+        <testResources>


Are you sure that you need to specify it?

sleshchenko · 2017-03-29T13:14:16Z

...ket-server/src/main/java/org/eclipse/che/ide/ext/bitbucket/server/BitbucketURLTemplates.java

        return BITBUCKET_2_0_API_URL + "/repositories/" + owner + "/" + repositorySlug + "/pullrequests";
    }

    @Override
+    public String updatePullRequestUrl(String owner, String repositorySlug, int pullrequestId) {


pullrequestId -> pullRequestId

sleshchenko · 2017-03-29T13:14:48Z

...xt-bitbucket-server/src/main/java/org/eclipse/che/ide/ext/bitbucket/server/URLTemplates.java

-    String pullrequestUrl(String owner, String repositorySlug);
+    String pullRequestUrl(String owner, String repositorySlug);
+
+    String updatePullRequestUrl(String owner, String repositorySlug, int pullrequestId);


pullrequestId -> pullRequestId

sleshchenko · 2017-03-29T13:15:39Z

...erver/src/main/java/org/eclipse/che/ide/ext/bitbucket/server/rest/BitbucketRequestUtils.java

     * @throws IOException
     *         if any i/o errors occurs
     * @throws BitbucketException
     *         if Bitbucket returned unexpected or error status for request
     */
-    public static String postJson(BitbucketConnection connection, String url, int success, String data) throws IOException,
+    public static String postJson(BitbucketConnection connection, String url, String data) throws IOException,


exceptions list is not formatted

sleshchenko · 2017-03-29T13:18:48Z

plugins/plugin-bitbucket/codenvy-plugin-bitbucket-ext-bitbucket-shared/pom.xml

+    <build>
+        <resources>
+            <resource>
+                <directory>src/main/java</directory>


Please be careful and do not add redundant sections which have the same values as default ones

GWT compilation won't work without that

It's not clearly. Thanks for explanation

sleshchenko · 2017-03-29T13:30:52Z

.../codenvy-plugin-webhooks-base/src/main/java/com/codenvy/plugin/webhooks/CloneUrlMatcher.java

+        final String projectLocation = source.getLocation();
+        final String projectBranch = source.getParameters().get("branch");
+
+        if (isNullOrEmpty(projectType) || isNullOrEmpty(projectLocation)) {


Can you explain link between cloneUrl and projectType?

I see. Actually It is not projectType but sourceType. Am I right? If yes, should you check that it equals to .git?

You are right, it is redundant, simplified the check

sleshchenko · 2017-03-29T13:42:43Z

.../codenvy-plugin-webhooks-base/src/main/java/com/codenvy/plugin/webhooks/CloneUrlMatcher.java

+     * @param branch
+     *         the branch that the project has to match
+     */
+    boolean isCloneUrlMatching(final ProjectConfigDto project, final String cloneUrl, final String branch);


What is profit to write final for variables here?

removed finals

sleshchenko · 2017-03-29T13:46:24Z

.../codenvy-plugin-webhooks-base/src/main/java/com/codenvy/plugin/webhooks/CloneUrlMatcher.java

+import static com.google.common.base.Strings.isNullOrEmpty;
+
+/**
+ * Matcher that checks clone url to be related to given project and branch.


Matcher that checks specified project to be related to given clone url and branch.
Would it better?

Looks better, changed

sleshchenko · 2017-03-29T14:51:59Z

...rver/src/main/java/org/eclipse/che/ide/ext/bitbucket/server/BitbucketServerURLTemplates.java

        return format(restUrl + REPOSITORY + "/pull-requests", owner, repositorySlug);
    }

+    @Override
+    public String updatePullRequestUrl(String owner, String repositorySlug, int pullrequestId) {


pullrequestId -> pullRequestId

sleshchenko

#2000 (comment)

bmicklea · 2017-03-31T13:05:03Z

@vinokurig - can you add the milestone please? I think it's 5.7

CODENVY-1921: BitbucketServer adaptations

5177f21

vinokurig requested review from skabashnyuk and sleshchenko March 28, 2017 15:07

bmicklea self-requested a review March 28, 2017 16:33

bmicklea added the kind/enhancement label Mar 28, 2017

bmicklea approved these changes Mar 28, 2017

View reviewed changes

bmicklea added this to the 5.6.0 milestone Mar 28, 2017

Igor Vinokur added 2 commits March 29, 2017 08:58

Merge branch 'master' of github.com:codenvy/codenvy into CODENVY-1921

b09e8cb

fixup! CODENVY-1921: BitbucketServer adaptations

cfa4de8

sleshchenko reviewed Mar 29, 2017

View reviewed changes

Igor Vinokur added 2 commits March 29, 2017 13:45

fixup! Merge branch 'master' of github.com:codenvy/codenvy into CODEN…

5742f26

…VY-1921

Merge branch 'master' of github.com:codenvy/codenvy into CODENVY-1921

50f83fc

skabashnyuk reviewed Mar 29, 2017

View reviewed changes

Merge branch 'master' of github.com:codenvy/codenvy into CODENVY-1921

fabd20f

sleshchenko reviewed Mar 29, 2017

View reviewed changes

fixup! CODENVY-1921: BitbucketServer adaptations

f2fb492

riuvshin removed this from the 5.6.0 milestone Mar 29, 2017

Merge branch 'master' of github.com:codenvy/codenvy into CODENVY-1921

134976f

sleshchenko reviewed Mar 29, 2017

View reviewed changes

sleshchenko suggested changes Mar 29, 2017

View reviewed changes

Igor Vinokur added 2 commits March 29, 2017 17:57

fixup! CODENVY-1921: BitbucketServer adaptations

50fa369

fixup! CODENVY-1921: BitbucketServer adaptations

92b6a5d

sleshchenko approved these changes Mar 29, 2017

View reviewed changes

Igor Vinokur added 3 commits March 29, 2017 18:43

Merge branch 'master' of github.com:codenvy/codenvy into CODENVY-1921

e3faeff

Merge branch 'master' of github.com:codenvy/codenvy into CODENVY-1921

099e6a0

Update maven version

343e571

vinokurig merged commit aaebfbb into master Mar 31, 2017

vinokurig deleted the CODENVY-1921 branch March 31, 2017 13:04

vinokurig added this to the 5.7.0 milestone Mar 31, 2017

JamesDrummond mentioned this pull request Apr 9, 2017

5.7.0 Milestone Overview #2028

Closed

8 tasks

vinokurig mentioned this pull request Jun 13, 2017

Code clean-up and refactoring of Bitbucket plugin #1438

Closed

CODENVY-1921: BitbucketServer adaptations #2000

CODENVY-1921: BitbucketServer adaptations #2000

Conversation

vinokurig commented Mar 28, 2017 • edited

What does this PR do?

What issues does this PR fix or reference?

Changelog

Release Notes

Docs PR

bmicklea commented Mar 28, 2017

bmicklea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skabashnyuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sleshchenko left a comment

Choose a reason for hiding this comment

bmicklea commented Mar 31, 2017

vinokurig commented Mar 28, 2017 •

edited