[JENKINS-43507] Migrate Bitbucket Plugin to Traits by stephenc · Pull Request #53 · jenkinsci/bitbucket-branch-source-plugin

stephenc · 2017-06-12T15:10:52Z

Downstream of:

[JENKINS-43507] Allow SCMSource and SCMNavigator subtypes to share common traits scm-api-plugin#36
[JENKINS-43507] Adapt to new trait based APIs branch-api-plugin#104
[JENKINS-43507] Allow SCMSource and SCMNavigator subtypes to share common traits git-plugin#494
[JENKINS-43507] Allow SCMSource and SCMNavigator subtypes to share common traits mercurial-plugin#103

Will not be merged until after equivalent changes to GitHub plugin have been pushed to a pull-request

@reviewbybees

P.S. I have tried to be as careful as possible making the changes step by step in order to ensure that the behaviour of the code being refactored remains unmodified for anyone upgrading from an existing configuration from older versions of the plugin. Unfortunately this required a large diff of changes in the end. If it is any consolation to reviewers, I will also be re-reviewing all this code before I am happy to merge... yes it is a 12k LOC pull request

…ations

… Jenkins

… of a PR

Also: - Adds support for building the merge commits of pull requests - Adds support for trusted revisions - Adds support for configuring Git extensions and Mercurial additional behaviours - Adds lots of tests (traits make code that was previously hard to test easier to test) Should: - Migrate existing configuration to the same effective configuration - Maintain backwards binary compatibility - Not maintain backwards source compatibility (if you wrote a plugin to the old APIs you will need to fix when you upgrade the plugin dependency)

ghost · 2017-06-12T15:11:45Z

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 · 2017-06-12T15:14:09Z

(build failure is just -SNAPSHOT dependency missing... will be fixed after I get the GitHub PR ready, if not before)

…eckbox

stephenc · 2017-06-12T15:29:52Z

Code coverage before:

Code coverage after:

stephenc · 2017-06-12T15:39:53Z

New UI for Branch Source showing the Bitbucket specific traits

stephenc · 2017-06-12T15:42:59Z

New UI for Navigator showing a legacy job upgraded:

stephenc · 2017-06-19T21:34:51Z

@rsandell I think I have addressed everything

rsandell

🐝

…rce-2.2.0-alpha-1

pascallap · 2017-06-20T15:52:36Z

src/main/resources/com/cloudbees/jenkins/plugins/bitbucket/BitbucketSCMNavigator/config.jelly

-    </f:advanced>
+<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form" xmlns:c="/lib/credentials" xmlns:scm="/jenkins/scm/api/form">
+  <j:choose>
+    <j:when test="${descriptor.serverUrlSelectable}">


Something seem wrong here.
We don't get the the drop down and/or unable to provide the Bitbicket Server url.

Forget that...
Found the config within the global configuration.

However.
the help tooltip

The list of servers is configured in the Manage Jenkins » Configure Jenkins › Bitbucket Endpoints screen. The list of servers can include both Bitbucket Cloud as well as Bitbucket Server instances.

Should be attach to the parent UI object.
Since the help is «invisibleEntry» on installation, it doesn't provide much help.

- Pick up updated git and mercurial dependencies - Pick up structs plugin 1.9 - Ensure consistent behaviour for new code

…rce-2.2.0-alpha-4

nfalco79 · 2017-06-27T18:24:03Z

I'm trying this PR because I need it like the water for the elephants.

I would just notify a change in behaviour or maybe a wrong help.
In the past the filter branch (now called "Filter by name with wildcards") matches the git name of the branch and based on that cute off the respective branches and PR folders.
For example:

filter: master feature/*
accept both feature/123 and PR-34 folder (that comes from feature/123)
reject PR-45 because it comes from demo branch (it does not matches filter)

Now the filter was applied on the folder name instead branch
For example:

filter: master feature/*
accept feature/123 folder
reject PR-34 that comes from feature/123

Actually I found a workaround to get it work

filter: master feature/* PR-*
accept feature/123 folder
accept PR-34 that comes from feature/123 (becuase matches PR-*)
accept PR-45 that comes from demo branch (originally rejected)

If this new behaviour is expected than fix the help of Filter by name that actually says:

Space-separated list of branch name patterns to consider. You may use * as a wildcard; for example: master release*

into

Space-separated list of patterns to consider. You may use * as a wildcard; for example: master release* PR-*

rsandell

🐝

nfalco79 · 2017-06-30T09:47:12Z

I'm using BB Cloud.

I want notify that with latest 3.4.0-alpha-4 the hook management is broken.
Our configuration is enable webhook in BB Cloud settings repo by repo because devOps team has not admin grants on repositories.
With previous (released) version each commit triggers a new build (branch event), now no more build are triggered. I have to setup the job to schedule index every 15 minutes to trigger builds (branch indexing).

vivek · 2017-06-30T17:23:51Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/endpoints/AbstractBitbucketEndpoint.java

+     * @return the URL of the endpoint.
+     */
+    @NonNull
+    public abstract String getServerUrl();


Is this unique across all bitbucket server endpoints?

Yes dedup on save

vivek · 2017-06-30T17:24:04Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/endpoints/AbstractBitbucketEndpoint.java

+     * @return the name to use for the end-point
+     */
+    @CheckForNull
+    public abstract String getDisplayName();


Why optional? Might be better to keep it as required, otherwise UI rendering of it could look awkward with blank names appearing in drop down. Same goes for duplicates.

Because we display ${displayName} (${serverUrl}) if provided or ${serverUrl} if not and the server URL is unique

Well, even in this case, if the intent is to display the name and url, why not have user enter them, it helps him identify these servers later on.

@i386 In BB mock-ups for blueocean, endpoint names appear in drop down box. I am enforcing name requirement via blueocean API and uniqueness (just like GitHub Enterprise), but there is mismatch with the ones created using classic UI. your thoughts?

@vivek is there a mismatch between how Bitbucket and Github handle endpoint names via classic too?

Why are names not unique? Seems an odd decision

@stephenc if the endpoint name is mandatory for Github can it also be mandatory for Bitbucket? Consistency in these areas makes a huge difference for us in Blue Ocean. Ping @vivek for details.

Also URLs can be crazy long. Makes it difficult to disambiguate them in a combo box.

Not a breaking change, because we can return for you the computed name if the name is not present or blank.

GitHub and BitBucket URLs are not going to be crazy long, because those services demand a root URL so it is basically just the hostname.

Users are going to want to see the URL that the display name corresponds to anyway...

Which GitHub is https://github3.example.com? is that Production or QA?

So we need to add the URL to the display name to help the user

Which GitHub is https://github3.example.com? is that Production (https://github3.example.com) or QA (https://github2.example.com)

This is because the user is used to the GIT URL.

They will know the host name

🐛 talk to some users - ask what their corporate URLs are like. LONG is very very common, and it doesn't have to be that long to make it useless in a drop down in any case. A unique name seems reasonable (in the classic UI it could compute one or make it the URL if they really don't want to type one in - if it is unique right?). If not long, then they are horrible and non memorable.

The users will not know the hostnames off by heart in most of the occasions (ie literally more than half - sure lots will know it).

As an example, I asked a friend from a "Big Company" (everyone has heard it, and uses it often) to rattle one off he used today: http://sj1010212072143.corp.[REDACTED]/ was his first one. That is surprisingly short but ugh...

Some treat them as a hash of something (I have seen stranger things...)

And making things consistent by breaking GHE behaviour is not the way to make things consistent of course (consistency isn't worth that price)

Which is why the name is "${displayName == null ? serverUrl : displayName + ' ('+serverUrl+')'}"

If we don't display the server url then we end up with people putting the server url in the display name, or worse putting the wrong server url in the display name.

If we provide the display name then we need to dedup on both otherwise the display name is meaningless or requires the server url anyway.

For the user to be able to trust the server url part of the display name, it has to be appended programmatically not relying on the admin.

ok in that case - I think the displayName is pointless. just use serverUrl which is both definitive and unique. If there is to be a "display Name" as above - it is more like an optional description and the UI can decide what to do with it, not lower level code. So for now - displayName is dead - serverUrl is the go. Having something optional which is sometimes used and sometimes not just muddles things - if serverUrl is enough, lets stick with just it. If people complain about crazy URLs in the future, then we can address it (easier to add, than to undo)

i386 · 2017-07-03T06:21:16Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/endpoints/AbstractBitbucketEndpoint.java

+     * @return the name to use for the end-point
+     */
+    @CheckForNull
+    public abstract String getDisplayName();


@vivek is there a mismatch between how Bitbucket and Github handle endpoint names via classic too?

Why are names not unique? Seems an odd decision

vivek · 2017-07-03T06:41:13Z

@i386 Yes in classic, GitHub makes name mandatory where as for bitbucket its optional. Both Github and Bitbucket require api url uniqueness. Neither require name to be unique.

michaelneale · 2017-07-03T09:52:17Z

-1 due to naming discussion above only. Unfortunately I don't have a day to day use of bitbucket at all (unlike some who own $TEAM shares ;) - so just looking at the code/API.

vilacides · 2017-07-04T15:47:52Z

pom.xml

-    </properties>
+  <properties>
+    <jenkins.version>1.642.3</jenkins.version>
+    <scm-api.version>2.2.0-alpha-1</scm-api.version>


Shouldn't this be 2.2.0-SNAPSHOT to match what is in master?

[FIXED JENKINS-39026] Add an abstract base class for simple ViewBranchFilter

stephenc added 16 commits May 22, 2017 11:53

[JENKINS-43433] Adapt to ChangeRequestSCMHead2

43b5668

[JENKINS-43433] Pick up scm api snapshot

301f8d0

Merge branch 'jenkins-44413' into jenkins-43507

74b8531

[JENKINS-43507] Add endpoints to global configuration

b8f9be3

[JENKINS-43507] Add endpoints tests

3530fca

[JENKINS-43507] Add endpoints to global configuration

dfd47ff

[JENKINS-43507] Tidy up implementation debt in BitbucketApi implement…

ee67e57

…ations

[JENKINS-43507] Align with endpoints expected serverUrls

fd9e115

[JENKINS-43507] Add backfill until JENKINS-42443 is available in base…

824dfd2

… Jenkins

[JENKINS-43507] Widen types of input parameter

0e1e96a

[JENKINS-43507] Add source context and request

88c4be3

[JENKINS-43507] Add navigator context and request

933a75a

[JENKINS-40475] Use a more resilient API call to determine the commit…

8f636d6

… of a PR

[JENKINS-43507] checkPathExists can take a branch OR a hash

95aac94

[JENKINS-43507] Add API to derive web links to endpoint configuration

50166af

[JENKINS-43507] Curses upon you IntelliJ with your "Reformat code" ch…

71b6cae

…eckbox

[JENKINS-43507] Fix formatting

103082f

rsandell approved these changes Jun 20, 2017

View reviewed changes

stephenc added 5 commits June 20, 2017 13:48

[JENKINS-43507] Pick up alpha-1

107f747

Merge branch 'master' into jenkins-43507

ec0fc5b

[maven-release-plugin] prepare release cloudbees-bitbucket-branch-sou…

6821f23

…rce-2.2.0-alpha-1

[maven-release-plugin] prepare for next development iteration

8b3b9eb

[JENKINS-43507] US people want to spell things incorrectly

17541eb

pascallap reviewed Jun 20, 2017

View reviewed changes

stephenc added 4 commits June 24, 2017 08:43

[JENKINS-43507] Fix trait drop-down list population

3fa5f43

[JENKINS-43507] Prepare for -alpha-4

b9a3016

- Pick up updated git and mercurial dependencies - Pick up structs plugin 1.9 - Ensure consistent behaviour for new code

[maven-release-plugin] prepare release cloudbees-bitbucket-branch-sou…

3c9bb0f

…rce-2.2.0-alpha-4

[maven-release-plugin] prepare for next development iteration

6f9a49f

rsandell approved these changes Jun 28, 2017

View reviewed changes

vivek reviewed Jun 30, 2017

View reviewed changes

i386 approved these changes Jul 3, 2017

View reviewed changes

vilacides reviewed Jul 4, 2017

View reviewed changes

stephenc added 4 commits July 5, 2017 18:49

[JENKINS-43507] Add missing continue

7ecc02d

Merge branch 'master' into jenkins-43507

b307aaf

Merge branch 'master' into jenkins-43507

425d7bd

[JENKINS-43507] Remove (now) useless dependency

ee66530

stephenc merged commit 2379c9b into jenkinsci:master Jul 5, 2017

stephenc deleted the jenkins-43507 branch July 5, 2017 21:46

abishek3876 mentioned this pull request Jul 13, 2017

Fix JENKINS-40465 #48

Closed

fengxx pushed a commit to fengxx/bitbucket-branch-source-plugin that referenced this pull request Mar 13, 2018

Merge pull request jenkinsci#53 from jenkinsci/jenkins-39026

ee698b5

[FIXED JENKINS-39026] Add an abstract base class for simple ViewBranchFilter

Conversation

stephenc commented Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 12, 2017

Uh oh!

stephenc commented Jun 12, 2017

Uh oh!

stephenc commented Jun 12, 2017

Uh oh!

stephenc commented Jun 12, 2017

Uh oh!

stephenc commented Jun 12, 2017

Uh oh!

stephenc commented Jun 19, 2017

Uh oh!

rsandell 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!

nfalco79 commented Jun 27, 2017

Uh oh!

rsandell left a comment

Choose a reason for hiding this comment

Uh oh!

nfalco79 commented Jun 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

vivek Jul 1, 2017 • 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!

michaelneale Jul 3, 2017 • 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!

vivek commented Jul 3, 2017

Uh oh!

michaelneale commented Jul 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

16 participants

stephenc commented Jun 12, 2017 •

edited

Loading

nfalco79 commented Jun 30, 2017 •

edited

Loading

vivek Jul 1, 2017 •

edited

Loading

michaelneale Jul 3, 2017 •

edited

Loading

michaelneale commented Jul 3, 2017 •

edited

Loading