[Bitbucket Server] Implements the GetMe method #128

panoramix360 · 2023-11-10T13:07:37Z

Summary

This is the first PR to begin the implementation of the Bitbucket Server in the Plugin.

It creates the client_server.go to integrate the Bitbucket Server methods to the Plugin, I implemented the first one GetMe this one does two requests to complete:

Gets the logged Bitbucket User using an OAuth endpoint
Uses the logged user name to GetUser using the go-bitbucket-v1 library

This flow is needed because the Bitbucket Server API doesn't have the same methods of the Bitbucket Cloud like /user that already accounts for the logged user.

Ticket Link

Github Issue

…oAmI

mattermost-build · 2023-11-10T13:07:41Z

Hello @panoramix360,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

codecov-commenter · 2023-11-16T04:21:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (bitbucket-server@3be0785). Click here to learn what that means.

Additional details and impacted files

@@                 Coverage Diff                 @@
##             bitbucket-server     #128   +/-   ##
===================================================
  Coverage                    ?   15.58%           
===================================================
  Files                       ?       15           
  Lines                       ?     2439           
  Branches                    ?        0           
===================================================
  Hits                        ?      380           
  Misses                      ?     2035           
  Partials                    ?       24

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mickmister

This is looking great @panoramix360! I left some comments on relocating BitBucket Server code to its own package, and managing the token for the client via the oauth2.Client instead of creating a http.Client directly. Otherwise LGTM 👍

server/client_server.go

mickmister · 2023-11-16T04:53:13Z

server/client_server_test.go

+		}
+
+		w.WriteHeader(http.StatusOK)
+		w.Write([]byte("mocked-user"))


You can do this to intentionally ignore the error, and avoid the lint issue:

Suggested change

w.Write([]byte("mocked-user"))

_ = w.Write([]byte("mocked-user"))

mickmister · 2023-11-16T04:56:28Z

server/client_server_test.go

+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		authHeader := r.Header.Get("Authorization")
+		if authHeader != "Bearer valid-token" {
+			w.WriteHeader(http.StatusUnauthorized)
+			return
+		}
+
+		w.WriteHeader(http.StatusOK)
+		w.Write([]byte("mocked-user"))


Can we have a similar test for the GetUser call? Maybe have one main "test" for ourBitbucketClient.GetMe, and have one of the test cases be a success case that returns a real user from the http server. Nice testing!

mattermost-build · 2023-11-27T01:02:45Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

panoramix360 · 2023-12-05T00:33:16Z

hey @mickmister

I added a Factory Pattern as we discussed, I think this approach is cleaner, can you review it again?

Meanwhile, I'll take a look and solve the other comments.

Can you take a look at some comments that I made as well?

Thanks!

server/client_server.go

mickmister · 2023-12-05T00:38:37Z

server/bitbucket_server/factory.go

+func GetBitbucketClient(clientType string, selfHostedURL string, selfHostedAPIURL string, apiClient *bitbucketv1.APIClient) (Client, error) {
+	if clientType == "server" {
+		return newServerClient(selfHostedURL, selfHostedAPIURL, apiClient), nil
+	}
+	return nil, fmt.Errorf("wrong client passed")
+}


If this package only creates a Bitbucket Server client, and not a Bitbucket Cloud client, what benefit do we get from having this clientType parameter?

Oh, the idea is to create the Bitbucket Cloud as well! I didn't insert this yet because we plan to do this more consistently on this issue.

But if you want, I can add the condition here. Let me know what you think!

I think the function to create the BB Cloud client will be in a separate package though like bitbucket_cloud. What do you think? We'll see how the code feels when instantiating either client from the main package

We can change the name of this package to just package bitbucket and have both on the same package.

so we have:

client.go: generic client with the necessary methods

client_server: concrete server client with the implementation of the Bitbucket Server

client_cloud: concrete cloud client with the implementation of the Bitbucket Cloud

What do you think of this approach?

If we think that everything to do with Bitbucket Server can/should fit in its own file, then this works well. But if there are multiple files for each client type, I think we should have separate packages for each client type. What do you think?

So either this that I think you're suggesting

client := bitbucket.NewBBServerClient()

or

client := bitbucket_server.NewClient()

server/client_server.go

…et server

mickmister · 2024-01-17T17:43:41Z

Hi @panoramix360, this is looking good 👍

Are you able to resolve the lint issues CI is reporting? Thank you!

panoramix360 · 2024-01-18T02:12:47Z

hey @mickmister, done!

I needed to change the name of the package, and it made more sense to call it bitbucketclient since it will have BitbucketServer and BitbucketCloud implementations.

mickmister

LGTM 👍 Just some comments for discussion. Great work @panoramix360!!

mickmister · 2024-01-26T17:31:32Z

server/bitbucketclient/factory.go

+	SelfHostedURL    string
+	SelfHostedAPIURL string


I've been thinking that this SelfHostedAPIURL is derived from SelfHostedURL (we just add some suffix to it), so maybe we can have just one config setting SelfHostedURL, and derive SelfHostedAPIURL at runtime?

mickmister · 2024-01-26T17:33:06Z

server/bitbucketclient/factory.go

+func GetBitbucketClient(clientType string, config ClientConfiguration) (Client, error) {
+	if clientType == "server" {
+		return newServerClient(config), nil
+	}
+	return nil, fmt.Errorf("wrong client passed")
+}


Just an idea, but I'm thinking having two different functions for creating the two clients may be more Go-idiomatic. So having a NewBBServerClient() function for instance to create the BB Server client

mickmister · 2024-01-26T17:33:54Z

server/bitbucketclient/client_server.go

+	BitbucketClient
+}
+
+func newServerClient(config ClientConfiguration) Client {


So we would make this function capitalized/exported if we want to expose it

panoramix360 added 8 commits October 17, 2023 21:56

feat: add go-bitbucket-v1

803c94c

feat: create the bitbucketv1Connect to connect to bitbucket server

18005d5

feat: adding the client_server to create the bitbucket server methods

608e55a

fix: fixes the ServeHTTP of the plugin

0226189

feat: transfering connect function to client_server and created GetWh…

4780f69

…oAmI

fix: api tests

78359db

test: simple tests on the client server

f5ee1bf

feat: craeting the GetMe method with struct types

dac3b50

mickmister reviewed Nov 16, 2023

View reviewed changes

mattermost-build added the Lifecycle/1:stale label Nov 27, 2023

panoramix360 added 4 commits November 28, 2023 20:59

feat: creating the bitbucket_server package

92655b0

feat: fix linting rules

0110aa0

feat: adding a bitbucket client factory

6028704

feat: adjustments to the access token

477c06e

mickmister reviewed Dec 5, 2023

View reviewed changes

panoramix360 added 3 commits December 6, 2023 11:25

feat: adding the logger as a configuration

7abb299

feat: wrapping errors in a custom message to have more tracking

1682d75

feat: using the oauth2.Client to make the requests inside the bitbuck…

c2c210c

…et server

mickmister self-requested a review January 9, 2024 17:03

tests the getWhoAmI

04549b3

panoramix360 added 2 commits January 17, 2024 23:06

feat: lint and renaming things

e393753

fix more lint problems

d790e0a

feat: removing the bitbucket server connect for now

d7fd5d9

mickmister approved these changes Jan 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bitbucket Server] Implements the GetMe method #128

[Bitbucket Server] Implements the GetMe method #128

panoramix360 commented Nov 10, 2023 •

edited by mickmister

Loading

mattermost-build commented Nov 10, 2023

codecov-commenter commented Nov 16, 2023 •

edited

Loading

mickmister left a comment

mickmister Nov 16, 2023

mickmister Nov 16, 2023

mattermost-build commented Nov 27, 2023

panoramix360 commented Dec 5, 2023

mickmister Dec 5, 2023

panoramix360 Dec 5, 2023 •

edited

Loading

mickmister Dec 6, 2023

panoramix360 Dec 6, 2023

mickmister Jan 26, 2024

mickmister commented Jan 17, 2024

panoramix360 commented Jan 18, 2024

mickmister left a comment

mickmister Jan 26, 2024

mickmister Jan 26, 2024

mickmister Jan 26, 2024

	w.Write([]byte("mocked-user"))
	_ = w.Write([]byte("mocked-user"))

[Bitbucket Server] Implements the GetMe method #128

Are you sure you want to change the base?

[Bitbucket Server] Implements the GetMe method #128

Conversation

panoramix360 commented Nov 10, 2023 • edited by mickmister Loading

Summary

Ticket Link

mattermost-build commented Nov 10, 2023

codecov-commenter commented Nov 16, 2023 • edited Loading

Codecov Report

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattermost-build commented Nov 27, 2023

panoramix360 commented Dec 5, 2023

Choose a reason for hiding this comment

panoramix360 Dec 5, 2023 • edited Loading

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

mickmister commented Jan 17, 2024

panoramix360 commented Jan 18, 2024

mickmister 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

panoramix360 commented Nov 10, 2023 •

edited by mickmister

Loading

codecov-commenter commented Nov 16, 2023 •

edited

Loading

panoramix360 Dec 5, 2023 •

edited

Loading