Bitbucket Server Root Context #113

nwinkler · 2018-01-09T08:22:20Z

I've finally found the time to take a look at the optional root context for self-hosted instances of Bitbucket Server. In our example, the Bitbucket server instance is not located at

https://bitbucket.example.com/scm/ppp/test-repo.git

but has an added /git/ before the scm part:

https://bitbucket.example.com/git/scm/ppp/test-repo.git

We had discussed this a while back in #83 (comment), but haven't had an implementation that took anything before the scm part into account.

I've refactored the Bitbucket logic to match the logic that was added for TFS, where the check for the scm part is now down from the back of the URL (the third to last item in the URL), instead of checking from the front.

No changes in the existing functionality, it just added support for self-hosted instances of Bitbucket Server that use an additional root context in the server deployment. Any optional path elements found before the scm match are now added to the final URL as part of the path transformation for Bitbucket Server. Please see the new unit tests for additional examples.

Please see the unit tests for more details. Basically, take the context parts before 'scm' and add them to the new URL before the other parts. Instead of taking fixed indexes for the path elements, base everything off the index of the found 'scm' path element.

derimagia · 2018-01-09T15:25:07Z

The idea for these type of things (which could exist in gitlab or github) is to use the git config to set this ala #92 - so not a bitbucket specific issue. Did you try this yet? We could merge the fix for bitbucket only although I think using the config here makes more sense.

nwinkler · 2018-01-10T07:51:14Z

Good points, but this is a different use case. For the GitLab case, you're trying to change a remote URL to a completely different URL, e.g. (from the unit tests):

Remote URL: git.example.com
Browser URL: gitlab.example.com

This means that the remote URL does not have all of the information required for creating the browser URL, so you need to source it in from somewhere else (git config in this case).

In the Bitbucket Server case that I added now, all of the information is in the remote URL, it just wasn't used before:

Remote URL: bitbucket.example.com/really/long/root/context/scm/ppp/test-repo.git
Browser URL: bitbucket.example.com/really/long/root/context/projects/ppp/repos/test-repo

Before my change, the really/long/root/context part was simply not used. The previous version of the code was checking for the scm element in position 0 of the path, whereas I have changed it now to look at the third to last position, allowing me to use the parts that come before scm for constructing the browser URL.

In this case, no external information needs to be sourced in from git config, since the remote URL already has all of the information.

I was able to remove one further if check from the code to clean it up a bit.

Does this make sense?

4U6U57

Assuming this is how Bitbucket servers handles URLs (which I have no experience in, so wouldn't know), this PR accomplishes the task given without affecting current functionality. Well commented and tested.

4U6U57 · 2018-01-11T02:44:18Z

test/git-open.bats

@@ -269,6 +269,35 @@ setup() {

 }

+
+@test "bitbucket server with different root context" {


TYPO: Please add colon here (bitbucket: ) so the CI output looks nice. Same on the following unit test.

Ah yes, thanks for noticing this. I also saw that some of the basic Bitbucket Server tests were duplicated in an earlier commit - I'll fix this at the same time as the naming...

Also removed some duplicated tests that were introduced during a previous merge.

nwinkler · 2018-01-11T07:21:34Z

I've fixed the naming of the test cases, and I've removed some duplicated tests for Bitbucket Server functionality as well. It looks like they were introduced as part of a previous commit/merge when the test suite was refactored.

derimagia · 2018-01-11T14:38:35Z

test/git-open.bats

+  assert_output "https://mybb.domain.com/root/context/projects/~first.last/repos/rrr/browse?at=develop" ||
+    assert_output "https://mybb.domain.com/root/context/projects/~first.last/repos/rrr/browse?at=refs%2Fheads%2Fdevelop" ||
+    assert_output "https://mybb.domain.com/root/context/projects/~first.last/repos/rrr/browse?at=refs/heads/develop"
+


#nitpick - remove the space?

Saw this too, but it's indentation for the chained ||.

We could possibly reduce duplicate strings by making use of assert_output --regexp, although it would be less readable. Like so:

assert_output --regexp "https://mybb.domain.com/root/context/projects/~first.last/repos/rrr/browse\?at=(refs(/|%2F)heads(/|%2F))?develop"

I've simply copied this part from the other existing unit tests, including the || chaining and the indentation.

Not a huge fan of the regexp check, due to readability...

derimagia · 2018-01-11T14:38:49Z

I see, so you want to change the url before the code here hits for bitbucket:

git-open/git-open

Lines 130 to 134 in 8caa9ad

    
           # Bitbucket server, which starts with 'scm' 
        
           # Replace the first element, 'scm', with 'projects'. Keep the first argument, the string 'repos', and finally the rest of the arguments. 
        
           pathargs=('projects' ${pathargs[1]} 'repos' "${pathargs[@]:2}") 
        
           IFS='/' urlpath="${pathargs[*]}" 
        
           providerBranchRef="/browse?at=$branch"

. So the other service we do custom work in is Team Foundation Server and it looks like this is similar to what that does. I'm good with this then 👍 .

nwinkler · 2018-01-30T07:39:15Z

Sorry to keep coming back to this - but is anything preventing this from getting merged? Let me know and I'll look into making any additional changes if required...

derimagia · 2018-02-04T16:32:44Z

👍

nwinkler · 2018-02-05T07:36:59Z

Thanks for merging!

Bitbucket Server Root Context

Removed an unnecessary if statement

119ada7

4U6U57 approved these changes Jan 11, 2018

View reviewed changes

4U6U57 reviewed Jan 11, 2018

View reviewed changes

Fixed test names to match existing tests

140edbb

Also removed some duplicated tests that were introduced during a previous merge.

derimagia reviewed Jan 11, 2018

View reviewed changes

derimagia merged commit beebcdb into paulirish:master Feb 4, 2018

nwinkler deleted the bb-server-context branch February 5, 2018 07:47

iblea pushed a commit to iblea/git-open that referenced this pull request Jun 17, 2024

Merge pull request paulirish#113 from nwinkler/bb-server-context

1d74d0c

Bitbucket Server Root Context

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Bitbucket Server Root Context #113

Bitbucket Server Root Context #113

Uh oh!

nwinkler commented Jan 9, 2018

Uh oh!

derimagia commented Jan 9, 2018

Uh oh!

nwinkler commented Jan 10, 2018

Uh oh!

4U6U57 left a comment

Uh oh!

4U6U57 Jan 11, 2018

Uh oh!

nwinkler Jan 11, 2018

Uh oh!

nwinkler commented Jan 11, 2018

Uh oh!

derimagia Jan 11, 2018

Uh oh!

4U6U57 Jan 11, 2018

Uh oh!

nwinkler Jan 11, 2018

Uh oh!

derimagia commented Jan 11, 2018

Uh oh!

nwinkler commented Jan 30, 2018

Uh oh!

derimagia commented Feb 4, 2018

Uh oh!

nwinkler commented Feb 5, 2018

Uh oh!

Uh oh!

		@@ -269,6 +269,35 @@ setup() {

		}


		@test "bitbucket server with different root context" {

Bitbucket Server Root Context #113

Bitbucket Server Root Context #113

Uh oh!

Conversation

nwinkler commented Jan 9, 2018

Uh oh!

derimagia commented Jan 9, 2018

Uh oh!

nwinkler commented Jan 10, 2018

Uh oh!

4U6U57 left a comment

Choose a reason for hiding this comment

Uh oh!

4U6U57 Jan 11, 2018

Choose a reason for hiding this comment

Uh oh!

nwinkler Jan 11, 2018

Choose a reason for hiding this comment

Uh oh!

nwinkler commented Jan 11, 2018

Uh oh!

derimagia Jan 11, 2018

Choose a reason for hiding this comment

Uh oh!

4U6U57 Jan 11, 2018

Choose a reason for hiding this comment

Uh oh!

nwinkler Jan 11, 2018

Choose a reason for hiding this comment

Uh oh!

derimagia commented Jan 11, 2018

Uh oh!

nwinkler commented Jan 30, 2018

Uh oh!

derimagia commented Feb 4, 2018

Uh oh!

nwinkler commented Feb 5, 2018

Uh oh!

Uh oh!