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

Add Bitbucket Server Support #516

Merged
merged 27 commits into from
Mar 15, 2018
Merged

Add Bitbucket Server Support #516

merged 27 commits into from
Mar 15, 2018

Conversation

azz
Copy link
Member

@azz azz commented Feb 17, 2018

Closes #515

Still a bunch of things to do but it is starting to take shape.

TODO:

  • Write tests
  • Implement getPullRequestDiff - Not sure if there's a way to download the raw patch...
  • Format in Markdown rather than HTML
  • Manual test build status
  • Manual test comment placement, update, delete
  • Documentation

}

getPullRequestDiff = async () => {
// TODO: possible?
Copy link
Member Author

Choose a reason for hiding this comment

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

Can't seem to find an endpoint to get a raw diff, but there's a structured version in the REST API. Could use that but would require changing some common code.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I'm reasonably sure the two underlying features that rely on this are:

  • Finding out the modified/created/deleted files
  • Generating metadata for a specific file (e.g. maybe a JSON diff, or showing before after etc)

There may be two different APIs that could get that kind of information from?

@orta
Copy link
Member

orta commented Feb 17, 2018

Wow, first of all, awesome 👍

@orta
Copy link
Member

orta commented Feb 17, 2018

Might be about an hour or two before I can think properly about this

@azz
Copy link
Member Author

azz commented Feb 17, 2018

No worries - I need to take a break so I'll pick this up again tomorrow.

Copy link
Member

@orta orta left a comment

Choose a reason for hiding this comment

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

Yep, this is definitely the right way to do this

href: string
name?: string
}[]
}
Copy link
Member

Choose a reason for hiding this comment

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

nice

// This is `danger.bitbucket_server`

/** The BitBucketServer metadata for your PR */
export interface BitBucketServerDSL extends BitBucketServerJSONDSL {}
Copy link
Member

Choose a reason for hiding this comment

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

if/once you start getting some helper functions in the bitbucket DSL, that's where this comes in handy.

export interface JIRAIssue {
key: string
url: string
}
Copy link
Member

Choose a reason for hiding this comment

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

This is cool

locked: boolean
author: BitBucketServerPRParticipant & { role: "AUTHOR" }
reviewers: (BitBucketServerPRParticipant & { role: "REVIEWER" })[]
participants: (BitBucketServerPRParticipant & { role: "PARTICPANT" })[]
Copy link
Member

Choose a reason for hiding this comment

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

Nice typing work

createComment = (comment: string) => this.api.postPRComment(comment)

// In Danger RB we support a danger_id property,
// this should be handled at some point
Copy link
Member

Choose a reason for hiding this comment

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

This is handled, I should remove the comment

yarn.lock Outdated
@@ -1371,9 +1371,9 @@ cli-spinners@^0.1.2:
version "0.1.2"
resolved "https://registry.yarnpkg.com/cli-spinners/-/cli-spinners-0.1.2.tgz#bb764d88e185fb9e1e6a2a1f19772318f605e31c"
Copy link
Member

Choose a reason for hiding this comment

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

ah, I bet this is me accepting greenkeeper PRs

@azz
Copy link
Member Author

azz commented Feb 18, 2018

Not nearly as pretty as GitHub 😢, but it works!

image

@azz
Copy link
Member Author

azz commented Feb 18, 2018

Since BitBucket server doesn't support HTML in markdown, I've replaced the table with a list of blockquotes, otherwise the warnings cannot be multi-line.

image

@azz
Copy link
Member Author

azz commented Feb 18, 2018

Build Status works:

image

image

@azz
Copy link
Member Author

azz commented Feb 18, 2018

get supportedPlatforms() {
return ["github"]
}

Noticed this in all the ci_sources. But it doesn't seem to be used?

@orta
Copy link
Member

orta commented Feb 18, 2018

re:

get supportedPlatforms()

Yeah, so only certain CI sources will support things like GitLab/BB/BBS - for example TeamCity supports GH/GitLab/BB/BBS in Ruby: code

So you can verify that the CI hooks up to the platform, but TBH, it might not need it, considering that the ENV vars added must mean that someone knows it will work, so maybe the abstraction isn't needed?

@orta
Copy link
Member

orta commented Feb 18, 2018

Re: the comment, tricky yeah. Another option is to do some funky markdown by adding line separators between the HTML content and the markdown

<table>
  <thead>
    <tr>
      <th width="5%"></th>
      <th width="95%" data-danger-table="true">Header</th>
     </tr>
  </thead>
  <tbody>
    <tr>
      <td>:+1:</td>
      <td>

some [markdown](/link)

    </td>
    </tr>
  </tbody>
</table>
Header
👍

some markdown

Which is the trick used in the gitlab markdown renderer for GitLab + BBS in Ruby

@azz
Copy link
Member Author

azz commented Feb 18, 2018

It doesn't seem to support any HTML at all. :(

@orta
Copy link
Member

orta commented Feb 19, 2018

@azz
Copy link
Member Author

azz commented Feb 23, 2018

(not sure what happened with that merge commit, will have to roll back)

@orta
Copy link
Member

orta commented Feb 23, 2018

The above message is an integration test that should only sending its messages to STDOUT btw, if you're wondering wha could have triggered it

@azz azz force-pushed the bitbucket-server branch 2 times, most recently from 0301946 to 15f7428 Compare February 24, 2018 05:56
return diffs.map(diff => ({
from: diff.source && diff.source.toString,
to: diff.destination && diff.destination.toString,
chunks: diff.hunks.map(hunk => ({
Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea what the difference between a hunk and a chunk is...

Copy link
Member

@orta orta Feb 24, 2018

Choose a reason for hiding this comment

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

All the pieces needed for a male model catalogue

@azz
Copy link
Member Author

azz commented Feb 24, 2018

I think all the functionality is done. I'll start writing tests tomorrow and look at documentation.

@azz
Copy link
Member Author

azz commented Feb 25, 2018

Added tests. 🔬

@azz
Copy link
Member Author

azz commented Feb 25, 2018

@orta know what's up with this build?
image

🔗 https://travis-ci.org/danger/danger-js/jobs/345868483

@orta
Copy link
Member

orta commented Feb 25, 2018

The TypeScript doesn't seem to compile, https://travis-ci.org/danger/danger-js/jobs/345868483#L593-L596

source/ci_source/providers/Nevercode.ts:46:15 - error TS6133: 'branchName' is declared but its value is never read.

Which cascades through some of the next tests.

@orta
Copy link
Member

orta commented Feb 25, 2018

Wow @azz, this is so much work 👍

@orta
Copy link
Member

orta commented Feb 25, 2018

I've got it green, and extended the DSL with some JSDocs

@azz
Copy link
Member Author

azz commented Feb 26, 2018

Awesome, I'll try and find some time in the next few days to write docs and give it a test on a project at work.

@orta
Copy link
Member

orta commented Feb 26, 2018

Cool cool, once you're happy with the docs in here I can go take it to the more gnarly getting started page

@azz
Copy link
Member Author

azz commented Mar 4, 2018

Had a busy week at work and didn't get a chance to try it out! I'll try again early this coming week.

@sunshinejr sunshinejr mentioned this pull request Mar 4, 2018
13 tasks
@orta
Copy link
Member

orta commented Mar 11, 2018

OK @azz - I've gone and done a docs run, and prepared for it on the website too ( https://gitlab.com/danger-systems/danger.systems/merge_requests/168 )

Want to give me a lint for it all and say whether this is good to go?

@zdnk
Copy link
Contributor

zdnk commented Mar 12, 2018

For some reason when installing from your branch I am not able to access danger command using yarn.

@orta
Copy link
Member

orta commented Mar 12, 2018

It's probably not being compiled by TypeScript

@zdnk
Copy link
Contributor

zdnk commented Mar 12, 2018

So how would I fix it? I am not JS dev, but really would like to adopt the JS version of danger with BBS support finally :)

@orta
Copy link
Member

orta commented Mar 12, 2018

I'd wait TBH if you're not a JS dev, it shouldn't be too long

@azz
Copy link
Member Author

azz commented Mar 14, 2018

👍 👍 👍

@orta orta merged commit 9bf089a into danger:master Mar 15, 2018
@orta
Copy link
Member

orta commented Mar 15, 2018

ok, let's try get this rolling

@orta orta changed the title WIP: Add Bitbucket Server Support Add Bitbucket Server Support Mar 15, 2018
@azz azz deleted the bitbucket-server branch March 15, 2018 08:51
@stigi stigi mentioned this pull request Apr 10, 2018
@langovoi
Copy link
Contributor

@azz i found that API /diff don't return diff for files in sub-sub-directory from root.

So modified_files, deleted_files and creates_files are wrong.

Do you have this problem with your Bitbucket Server? I could not find any information or issues about this bug/behavior.

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.

4 participants