Skip to content
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

Migration to bitbucket api v2 #321

Closed
wants to merge 6 commits into from

Conversation

Xen3r0
Copy link

@Xen3r0 Xen3r0 commented Aug 13, 2019

@Xen3r0 Xen3r0 mentioned this pull request Aug 13, 2019
Copy link
Member

@dregad dregad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried a quick test and it failed to import anything. I tried both with master and * in branches repo config,

I don't have time to investigate further right now, but please have a look at the inline comments below.

Also, I would suggest since API 1.0 is gone, to remove protected $api_url_10 (line 18). And since there is only one API being used, it may be worth getting rid of the _20 suffixes.

Finally, I noted some whitespace issues in the GitHub diffs, most likely because you indented with spaces instead of tabs.

SourceBitBucket/SourceBitBucket.php Outdated Show resolved Hide resolved
SourceBitBucket/SourceBitBucket.php Outdated Show resolved Hide resolved
@Xen3r0
Copy link
Author

Xen3r0 commented Aug 14, 2019

Can you review again ?

Copy link
Member

@dregad dregad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update.

As far as I can tell, both full and latest import functions works fine now, both with all (*) and comma-delimited list of branches.

I did not test integration with webhook to process commits though.

@semanticfire
Copy link

I've tested the PR in a full integration situation, bitbucket commit initializing a mantis update, both * and comma separated lists work as expected

@dregad
Copy link
Member

dregad commented Sep 6, 2019

Awesome, many thanks for taking the time to test !

@dregad dregad closed this in 5b9a93b Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants