-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Bitbucket Self-Hosted URL #118
Add support for Bitbucket Self-Hosted URL #118
Conversation
…aces the locations where the url is used
hey @mickmister and @jprusch! Just to let you guys know that I created the draft. Things missing on this PR that I plan to implement:
@mickmister Since you are probably more familiar with the code, I saw that on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great @panoramix360! Thanks for your effort on this 👍
Just a few comments on my first pass here
I was thinking that maybe we will need to have this on configuration too? or am I wrong?
Indeed we'll need to change any URL that communicates with BitBucket's API. You can think of the plugin as a closed off system that only has access to the self-hosted BitBucket, so all communication needs to be done exclusively with that instance.
plugin.json
Outdated
} | ||
] | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that all files should end in a newline. This red indicator shows that there is not one here. If you use VSCode, you can configure the editor to do this automatically on save with these settings:
"files.insertFinalNewline": true,
"files.trimFinalNewlines": true,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, thank you! ;) fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this file and docker-compose.yml
both are missing final new line
hey @mickmister! thank you for your comments, fixed all of them :D A question about the API Url that I mentioned, do you think we will need to create a second property on the Plugin Configuration to have the API Url configured? I don't know if it's possible to infer these from the Please, let me know your thoughts. |
@panoramix360 Yes this makes sense. We've done the same pattern for other plugins like the zoom plugin https://github.com/mattermost/mattermost-plugin-zoom/blob/de8de086a243bef494a2df4116c7e2a4e5ef155a/plugin.json#L37 |
Thanks @mickmister! I'll add it! I created the |
Awesome work with the docker setup @panoramix360 🎉 |
hey @mickmister! I'm trying to test with the bitbucket locally, but I'm having some trouble in configuring it. I followed the tutorial on I think I configured everything:
But it seems that the plugin is not being able to start it. When I save a new config on the Mattermost Bitbucket page, I receive this log on the server:
Maybe I'm missing something. I noticed that when I try to type |
@panoramix360 Do you happen to be using an M1 Mac or possibly linux ARM? This project is currently not updated to support M1 Mac. In order to fix this, we'll need to copy some lines over from the plugin starter template: Ideally we would synchronize the starter template project with this project, to make it more in sync than just these blocks here. But I'm fine making just this change for M1 with this for the purpose of this PR. Please let me know what you think 👍 If this is indeed the issue for you, I'll create a ticket to update the wording of that error to be more obvious about the M1 issue. |
Nice catch!! yes, I'm using an M1 Mac! Thank you for that, I'd eventually waste a lot of time until I figured it out it was this haha I'll update the files then in this PR, thanks! |
2ec7b3a
to
b0e99a6
Compare
hey @mickmister! I was able to make the plugin work and be enabled now, thanks for the tip! I added the I already configured everything (or so I think haha), but it seems that the Webhook is not working correctly. Can you try to test on your side as well? I'm having the following error, I'm not sure is related but it seems so:
I didn't configure the Bitbucket API Self-Hosted URL yet on the Plugin Configurations, because I wasn't able to discover what is this URL for the Bitbucket locally. Since this property is probably just used to add links to the messages I believe it's not the problem. I'll investigate more to try to tackle the error. I think that the current code solution is already sufficient to solve the issue :D It's just missing these improvements:
|
Hi @panoramix360, I should have some time to test this tomorrow. Here are some thoughts on your last comment:
I think those errors are unrelated to the plugin. Nothing to worry about.
Your Mattermost server will need to be reachable from the BitBucket server for webhook requests sent to Mattermost. In order to do this, you can use a tool like https://ngrok.com. Running
This article shows this as an example API URL:
So maybe it would be http://localhost:7990/rest/api/1.0 in the case of the local docker environment?
I'm not sure what this means. If I understand correctly, this needs to be set for all API requests right? Thanks so much for your efforts on this @panoramix360. Please let me know your thoughts on the above 👍 |
Also note that I submitted a PR to your branch to add some files related to development with Gitpod panoramix360#1 |
…erver Add gitpod files for docker compose
hey @mickmister! Thank you for the PR and feedback!
Ok! Thank you!
I read something about Ngrok on the Mattermost Docs, I figured it could be something related to that. I already used Ngrok in the past, I'll install it and try to make the port forwarding, let's see. I think that's what I need to finally finish the testing.
Oh nice find! I'll test this URL to see if it's correct and I'll get back to you.
Yep, you are right, I thought that the Bitbucket API was used on links inside plugin messages on the chat, but I'm wrong. Just looked through the code and actually, it uses the API URL to compose the URLs for pulling PRs and other data. Just approved your PR :D! I'll wait for your tests as well then, ty! |
Just added the URL validation on the With that, I believe it's not more a Draft 🕺 |
Awesome thanks @panoramix360! From your testing, can you please describe what you've determined to be working, and what seems to not be working? |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## bitbucket-server #118 +/- ##
====================================================
+ Coverage 15.03% 15.17% +0.13%
====================================================
Files 13 13
Lines 2301 2346 +45
====================================================
+ Hits 346 356 +10
- Misses 1936 1969 +33
- Partials 19 21 +2
☔ View full report in Codecov by Sentry. |
hey @mickmister! I'm going to test the bitbucket connection and try to test many hooks, especially the ones that touch the code that I changed. Let me know if you think we should test other things. |
hey @mickmister Committed some code to update the code that we managed to find out about the Bitbucket Server on our talk! Can you take a look? I tried to do some testing today and tried other OAuth Scopes but I didn't manage to make it work, unfortunately. I found this doc, but it's not with the Bitbucket Server scopes reference. If you have any tips, just let me know! |
d752a67
to
150290a
Compare
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work @panoramix360 🎉 I think this is ready to merge with regards to the initial effort of supporting BB server
I have just a few suggestions here. Let me know what you think and we can move forward 👍
plugin.json
Outdated
} | ||
] | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this file and docker-compose.yml
both are missing final new line
if config.IsBitbucketSelfHosted() { | ||
authURL.Path = path.Join(authURL.Path, "rest", "oauth2", "latest", "authorize") | ||
tokenURL.Path = path.Join(tokenURL.Path, "rest", "oauth2", "latest", "token") | ||
scopes = append(scopes, "PUBLIC_REPOS", "REPO_READ", "ACCOUNT_WRITE") | ||
} else { | ||
authURL.Path = path.Join(authURL.Path, "site", "oauth2", "authorize") | ||
tokenURL.Path = path.Join(tokenURL.Path, "site", "oauth2", "access_token") | ||
scopes = append(scopes, "repository") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! 👍
@panoramix360 It looks like there are a few linting errors. Can you please take a look at the errors below? We can merge once these are solved 🚀
|
@mickmister The first lint problem I fixed. The second one is trickier because it's added on a map with a type that expects this third argument, what we can do in this case? EDIT: Fixed with an underscore haha sorry I'm relatively new with GoLang Lint |
Will try to auto merge this PR once all tests and checks are passing. This might take up to an hour. |
3be0785
into
mattermost-community:bitbucket-server
Thanks @panoramix360! We've merged the PR into the |
Summary
This PR plans to create a new field on the Plugin Configurations to let the user set a self-hosted Bitbucket URL instead of only having the option to use the fixed one:
https://bitbucket.org/
.Ticket Link
Fixes mattermost/mattermost#24188