-
Notifications
You must be signed in to change notification settings - Fork 139
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 Context path #747
Conversation
@fbuchmeier-abi would you be able to test this fix? There is no patch release planned yet, so it may take time for this to be officially released. You should be able to build notification-controller container image by running |
Hi @darkowlzz , thank you very much for the quick fix. I will test it tomorrow and let you know how it works. |
Hi, sorry for the late response. I was able to test the fix and it is working fine in our environment with the following configuration:
|
@fbuchmeier-abi, @fbuchmeier-abi : I have now added code to support all of the scenarios. Here is how it works: Look at the last /scm/ in the path (after host). Anything, if present, on left of it is context path. Anything right of it is project/repo. We finally build api path by using the context, project and repo values. We don't care what's on the left of last /scm/ i.e context path may contain any depth or may contain /scm/ @fbuchmeier-abi Could you please test this new code in your env? I have tested in mine for all different scenarios and it works fine. @darkowlzz : While working on this fix, I realized there were some unused variables being passed in functions. I added the fix for it in this PR. Is it okay to club those fixes with this change or should I revert these and submit a separate PR to fix these? |
@gdasson yes, seems good to me. Thanks. |
@gdasson I've built and deployed commit a3d456c in our env and it is working as expected, thanks! I've used the following provider config:
|
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.
The fix looks good to me but I left a suggestion for an overall improvement of the provider related to URL paths. Please have a look.
b5ea6c1
to
fa5f5a0
Compare
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.
The new implementation looks great. Thanks.
Left a few minor comments.
Signed-off-by: Gaurav Dasson <gaurav.dasson@gmail.com>
@darkowlzz : Fixed all of the review comments. Please review. Thx. |
@gdasson can you please rebase with upstream main |
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.
LGTM! Thanks @gdasson
Add support for Bitbucket Context path
Fix: #742