[JENKINS-39355 Follow-up] Unpicking some of the replicated hacks from… by stephenc · Pull Request #23 · jenkinsci/bitbucket-branch-source-plugin

stephenc · 2016-10-28T16:48:13Z

… the GitHub Org Folders plugin

@amuniz @reviewbybees

… the GitHub Org Folders plugin

- Also add bitbucket links and icons

…errupt checking

amuniz · 2016-11-23T19:32:08Z

Do you plan to continue working on this?

I'd like to review and merge #25 but it will generate non-trivial conflicts (specially as you are changing the representations of SCMHead here).

stephenc · 2016-11-23T20:19:25Z

Still working on this

…SCM API 2.0+

stephenc · 2016-12-15T16:19:59Z

@amuniz ready for review now

stephenc · 2016-12-15T16:21:33Z

I have tested this against BitBucket and BitBucket Enterprise.

jglick

Possible readResolve mistake.

jglick · 2016-12-15T16:44:57Z

pom.xml

            <groupId>org.jenkins-ci.plugins</groupId>
            <artifactId>git</artifactId>
-            <version>2.3.5</version>
+            <version>2.6.1-SNAPSHOT</version>


Is this waiting for a specific PR? If so, mention it here in a comment.

downstream of jenkinsci/git-plugin#454

jglick · 2016-12-15T16:45:16Z

pom.xml

        </dependency>
+        <dependency>
+            <groupId>org.jenkins-ci.plugins</groupId>
+            <artifactId>branch-api</artifactId>


Downstream of jenkinsci/branch-api-plugin#60

jglick · 2016-12-15T16:46:05Z

pom.xml

-            <groupId>org.jenkins-ci.plugins</groupId>
-            <artifactId>branch-api</artifactId>
-            <version>1.10.2</version>
+            <version>2.0.1-SNAPSHOT</version>


Downstream of jenkinsci/scm-api-plugin#19

jglick · 2016-12-15T16:49:51Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketRepoMetadataAction.java

+/**
+ * Invisible property that retains information about GitHub repository.
+ *
+ * @author Kohsuke Kawaguchi


Really? I think this is a ship of Theseus.

jglick · 2016-12-15T16:52:15Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketSCMSource.java

+                                        + pull.getSource().getBranch().getName());
+                    } else {
+                        e.printStackTrace(
+                                listener.error("Can not resolve hash: [%s]%n", pull.getSource().getCommit().getHash()));


jglick · 2016-12-15T17:03:14Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/SCMHeadWithOwnerAndRepo.java

-            return Integer.parseInt(metadata.getId());
-        } else {
-            return null;
+            return new PullRequestSCMHead(repoOwner, repoName, super.getName(), new BitbucketPullRequest() {


Do not use an anonymous inner class here; it will not serialize well.

It is not stored, only data extracted from it, but i'all double check

amuniz · 2016-12-15T17:06:50Z

2 test failures, related? Not clear to me after all this refactoring.

stephenc · 2016-12-15T18:25:28Z

Possibly tests need updating

stephenc · 2016-12-15T22:58:30Z

pom.xml

            <groupId>org.jenkins-ci.plugins</groupId>
            <artifactId>mercurial</artifactId>
-            <version>1.54</version>
+            <version>1.58-SNAPSHOT</version>


Downstream of jenkinsci/mercurial-plugin#92

stephenc · 2016-12-15T23:10:14Z

Currently failing until a snapshot of mercurial plugin is deployed

amuniz

After a more or less blind review (as I didn't check what changed upstream in scm-api and branch-api), it looks ok to me, but I want to try it working before approving.

amuniz · 2017-01-02T13:46:55Z

README.adoc

+=== Notes
+
+* Unlike GitHub, in BitBucket, https://bitbucket.org/site/master/issues/4828/team-admins-dont-have-read-access-to-forks[team admins do not have access to forks].
+This means that when you have a private repository, or a private fork of a public repository, the team admin will not be able to see the PRs within the fork.


In general the user configured for scanning needs to have read permission in all repositories intended to be scanned.

Including the forks where a PR to be scanned lives.

amuniz · 2017-01-02T13:50:15Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketApiConnector.java

-                StandardUsernameCredentials.class, context, ACL.SYSTEM, bitbucketDomainRequirements()));
+    public StandardListBoxModel fillCredentials(StandardListBoxModel result, SCMSourceOwner context) {
+        result.includeMatchingAs(
+                context instanceof Queue.Task


Is SCMSourceOwner a Queue.Task under some condition?

All current implementations of SCMSourceOwner are a Queue.Task but there is not requirement that they be so

amuniz · 2017-01-02T13:54:01Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketRepoMetadataAction.java

+import jenkins.scm.api.metadata.AvatarMetadataAction;
+
+/**
+ * Invisible property that retains information about GitHub repository.


GitHub -> Bitbucket

amuniz · 2017-01-02T13:57:16Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketSCMNavigator.java

    }

-    @Extension 
+    private String bitbucketServerUrl() {


This method name is misleading as it can return the Bitbucket Cloud or Bitbucket Server URL. I think bitbucketUrl would be preferable.

This should be fixed before merge.

amuniz · 2017-01-02T13:57:57Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketSCMNavigator.java

+    @Override
+    public List<Action> retrieveActions(@NonNull SCMNavigatorOwner owner,
+                                        @CheckForNull SCMNavigatorEvent event,
+                                                                @NonNull TaskListener listener)


Weird indentation

amuniz · 2017-01-02T14:04:44Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketSCMSource.java

        if (bitbucket.isPrivate()) {
            List<? extends BitbucketPullRequest> pulls = bitbucket.getPullRequests();
            for (final BitbucketPullRequest pull : pulls) {
+                checkInterrupt();


Why is this explicit check needed? Is this running in a different thread from the main scan thread?

You are supposed to call this method periodically so that people can abort scanning / indexing

amuniz · 2017-01-02T14:10:16Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BranchSCMHead.java

+
+/**
+ * {@link SCMHead} for a BitBucket branch.
+ * @since FIXME


2.0.0-beta-1?

tradition is to replace the FIXMEs after merge

amuniz · 2017-01-02T14:16:23Z

...dbees/jenkins/plugins/bitbucket/client/pullrequest/BitbucketPullRequestValueDestination.java

+import org.codehaus.jackson.annotate.JsonProperty;
+
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class BitbucketPullRequestValueDestination implements BitbucketPullRequestDestination {


Isn't BitbucketPullRequestValueRepository reusable for this purpose?

amuniz · 2017-01-02T14:19:52Z

src/test/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketClientMockUtils.java

+        branch = new BitbucketCloudBranch();
+        branch.setName("branch1");
+        destination.setBranch(branch);
+        pr.setDestination(destination);


Missing call to destination.setRepository(...) - probably not used now in tests, but expected to be set anyway.

amuniz · 2017-01-02T14:22:44Z

src/test/java/com/cloudbees/jenkins/plugins/bitbucket/SCMNavigatorTest.java

            this.listener = listener;
        }

+        @NonNull


@NonNull and then return null, something is wrong here (maybe not affecting to tests as a lot of code is mocked)

pascallap · 2017-01-06T14:46:24Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketSCMSource.java

+            result.add(new BitbucketRepoMetadataAction(r));
+        }
+        String serverUrl = StringUtils.removeEnd(bitbucketServerUrl(), "/");
+        result.add(new BitbucketLink("icon-bitbucket-repo", serverUrl + "/" + repoOwner + "/" + repository));


The link pattern doesn't work with Bitbucket Server,
Here is a suggestion:

String serverUrl = StringUtils.removeEnd(bitbucketServerUrl(), "/"); if (StringUtils.isNotEmpty(bitbucketServerUrl)) { result.add(new BitbucketLink("icon-bitbucket-repo", serverUrl + "/projects/" + repoOwner + "/repos/" + repository)); }else{ result.add(new BitbucketLink("icon-bitbucket-repo", serverUrl + "/" + repoOwner + "/" + repository)); }

@stephenc what about this?

pascallap · 2017-01-06T15:36:46Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketSCMSource.java

+            PullRequestSCMHead pr = (PullRequestSCMHead) head;
+            branchUrl = pr.getRepoOwner() + "/" + pr.getRepository() + "/branch/" + pr.getBranchName();
+        } else {
+            branchUrl = repoOwner + "/" + repository + "/branch/" + head.getName();


Link generated doesn't work for Bitbucket Server.
Now the question is, should it link to the «compare» or «source»/«browse» screen?
And in the case of a PullRequest, link back to the PullRequest or same «branch» link?

In the case of the pull request, linking to the pull request itself would make sense.

In the spots where Jenkins displays the COMMIT ID, such as the Changes page of a given job, I think linking to the specific Commit would be lovely.

https://bitbucket.mycorp.com/projects/my-prj/repos/my-repo/commits/my-commit-id

amuniz

No major changes required but at least this two little details should be fixed.

amuniz · 2017-01-12T19:12:28Z

src/main/resources/com/cloudbees/jenkins/plugins/bitbucket/Messages.properties

@@ -1,5 +1,13 @@
+BitbucketLink.DisplayName=Bitbucket
+BitbucketSCMNavigator.UncategorizedSCMSourceCategory.DisplayName=Repositories
+BitbucketSCMSource.UncategorizedSCMHeadCategory.DisplayName=Repositories


I think this should be Branches, right?

Yeah, either this or the previous line is wrong, checked in the UI.

amuniz · 2017-01-12T19:13:30Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketSCMNavigator.java

    }

-    @Extension 
+    private String bitbucketServerUrl() {


This should be fixed before merge.

stephenc · 2017-01-12T21:49:07Z

@amuniz should be addressed

amuniz · 2017-01-13T08:55:32Z

👍 Only pending a release of mercurial-plugin, holding merge until that.

tknerr · 2017-01-13T10:36:33Z

@stephenc @amuniz very cool stuff!

btw: does that mean we with this PR we get backlinks to the pull request / commit as well (e.g. when the changes are displayed in jenkins)?

ghost · 2017-01-13T14:04:46Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

stephenc added 2 commits October 28, 2016 17:47

[JENKINS-39355 Follow-up] Unpicking some of the replicated hacks from…

0d5b3a4

… the GitHub Org Folders plugin

[JENKINS-39355 Follow-up] More dead code removed

a2da353

amuniz added the work-in-progress label Oct 28, 2016

stephenc added 3 commits November 1, 2016 12:30

[JENKINS-39355 Follow-up] Remove data duplication from BranchSCMHead

76c087f

[JENKINS-39355 Follow-up] Sync with latest scm-api improvements

b932be3

- Also add bitbucket links and icons

[JENKINS-39355] Switch to the utility checkInterrupt() method for int…

a0adbe0

…errupt checking

amuniz mentioned this pull request Nov 23, 2016

[JENKINS-36283] custom pr behavior #25

Closed

6 tasks

stephenc added 2 commits December 5, 2016 18:45

Pick up cloudbees-folders 5.14

6373fb6

[JENKINS-39355 Follow-up] Lots of changes to make this work with the …

6899192

…SCM API 2.0+

jglick suggested changes Dec 15, 2016

View reviewed changes

stephenc added 5 commits December 15, 2016 22:09

Fix mock

e1f33de

Tidy up data migration

74f9d75

No need to retain author tag

8d83333

Typo

7a902d4

We need the mercurial plugin update too

99c2d3c

stephenc commented Dec 15, 2016

View reviewed changes

Prepare for beta-1

80adc94

stephenc force-pushed the jenkins-39355-follow-up branch from b40537c to 80adc94 Compare December 16, 2016 13:45

Pick up 2.6.2-beta-1 of git plugin

93141b3

amuniz reviewed Jan 2, 2017

View reviewed changes

amuniz added the needs-manual-testing label Jan 2, 2017

pascallap reviewed Jan 6, 2017

View reviewed changes

Merge branch 'master' into jenkins-39355-follow-up

049155b

amuniz requested changes Jan 12, 2017

View reviewed changes

[JENKINS-39355 Follow-up] Address code review comments

991057b

stephenc added 2 commits January 12, 2017 22:05

remove junk line

604e7b4

Findbugs catches bugs... who knew

68a7ec4

amuniz approved these changes Jan 13, 2017

View reviewed changes

Eernie mentioned this pull request Jan 13, 2017

[JENKINS-38748] Add auto register for BB server #21

Merged

amuniz removed needs-manual-testing work-in-progress labels Jan 13, 2017

amuniz merged commit 5c1b33f into jenkinsci:master Jan 16, 2017

stephenc deleted the jenkins-39355-follow-up branch January 16, 2017 15:05

Conversation

stephenc commented Oct 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amuniz commented Nov 23, 2016

Uh oh!

stephenc commented Nov 23, 2016

Uh oh!

stephenc commented Dec 15, 2016

Uh oh!

stephenc commented Dec 15, 2016

Uh oh!

jglick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenc Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amuniz commented Dec 15, 2016

Uh oh!

stephenc commented Dec 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenc commented Dec 15, 2016

Uh oh!

amuniz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenc commented Oct 28, 2016 •

edited

Loading

stephenc Dec 15, 2016 •

edited

Loading