-
-
Notifications
You must be signed in to change notification settings - Fork 371
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
Adds support for GitLab CI #855
Conversation
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.
Honestly, nothing really blocking from my opinion - it'll need to be green, but it's not in too bad of a shape at all.
@@ -251,7 +251,7 @@ export class Executor { | |||
await this.platform.deleteMainComment(dangerID) | |||
const previousComments = await this.platform.getInlineComments(dangerID) | |||
for (const comment of previousComments) { | |||
if (comment) { | |||
if (comment && comment.ownedByDanger) { | |||
await this.deleteInlineComment(comment) |
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.
interesting find, I wonder if it was failing and being caught
Not too sure what to do about the failed builds at this point, its failing on the Node 8.x tests since the GitLab module requires 10+ |
Are those tests easily scoped to just a few files which we can force to only run on Node 10? |
@orta I think that it needs the 5.x branch. I just submitted a PR to the repo to bring back 8.x support incase that is an easier resolution, alternatively is there a specific reason to continue to support 8 in Danger? |
Not really a reason, but a new dependency forcing us to remove support for older node versions in danger is pretty meh IMO (especially as it's still in long term support for the next ~6 months) |
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.
Hey, neat, thanks for picking this up @bigkraig!
It's pretty light on tests (that's next on my TODO), but that doesn't need to be a blocker. I also haven't tested across cloud vs self-hosted, and that various CE vs EE endpoints send slightly different responses.
So, I'm not 100% confident in this yet, but I think "good enough" will be enough to find out what's broken, and we can iterate on that!
Before I go and spend more time trying to chase down why my
I get that it's not a function (code is not committed yet but i'm setting it everywhere AFAIK.
|
Close, it should be a promise: |
Note that I didn't implement the |
- providing DANGER_GITLAB_API_TOKEN is now sufficient to use the GitLab platform - if no DANGER_GITLAB_HOST is provided default to GitLab cloud - add support for CE/EE without SSL
I've fixed the merge conflict, just to restore the CI |
updateOrCreateComment is using its own logic for detecting danger notes, change it to use getDangerNotes getDangerNotes is only used by methods that update the main comment, change it to include the optimisations that were in updateOrCreateComment
Latest version available using |
I wouldn't say it's ready to ship to the main release until we have added
more tests. Also, there are a few features missing and the documentation
needs updated.
Could we create a seperate release for this branch?
…On Mon, 3 Jun 2019, 22:15 Orta, ***@***.***> wrote:
Heyyyyy, this is green!
@bigkraig <https://github.com/bigkraig> @jamime
<https://github.com/jamime> - I'm happy to get this in and shipped when
you are?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#855?email_source=notifications&email_token=AARYL2VYVRFBSBLVI7HERRLPYWCXFA5CNFSM4HENMUQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODW2W3LI#issuecomment-498429357>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARYL2S7TOYV6GQ6MFVVPSTPYWCXFANCNFSM4HENMUQA>
.
|
When danger-pr is called with a GitLab URL with no DANGER_GITLAB_API_TOKEN use the GitLab provider instead of defaulting to GitHub
Implementing getFileContents adds suppport for - danger.gitlab.util.fileContents - danger.git.JSONDiffForFile - danger.git.JSONPatchForFile
@orta Testing is still limited. Documentation is still limited, mainly around how to contribute, however now that danger-pr works correctly with a GitLab URL the existing documentation is pretty platform agnostic. |
Those look like TypeScript errors and not node specific ones - I re-ran CI to see if there was something odd going on there, I feel like I've seem them before and may be periodic |
@orta when I run danger-pr with a gitlab URL it hangs for a few mins after outputting the results. Any idea why it's not exiting correctly? Works fine with GitHub URLs I looked at implementing getStructuredDiffForFile but GitLab differs from both BitBucket and GitHub, it instead gives a hybrid of the two results. The metadata is extracted but the changes are still displayed as a diff. Left to implement:
I'm happy to ship and raise another PR for these features when I get some more time. Now that fileContents and JSONDiffForFile works I think it will meet most users needs. I still cant reproduce the breaking build locally. |
I'd try running danger-pr with Re: CI, I was wondering if somehow on that specific instance of CI TypeScript gets updated |
@orta This is the output from the log, when compared to the log from GitHub it is identical. The only difference is the process doesn't exit.
✔️ Travis is fixed. |
@orta can you clear the cache on Travis and click off the build again please. |
nockAuthor: Pedro Teixeira Description: HTTP server mocking and expectations library for Node.js Homepage: https://github.com/nock/nock#readme
@types/nockAuthor: Unknown Description: TypeScript definitions for nock Homepage: http://npmjs.com/package/@types/nock
|
Created | over 6 years ago |
Last Updated | 4 days ago |
License | MIT |
Maintainers | 3 |
Releases | 110 |
Direct Dependencies | form-data , humps , ky , ky-universal , li , query-string , randomstring and universal-url |
Keywords | api, browser, es5, es6, gitlab and ky |
New dependencies added: gitlab
, @types/nock
and nock
.
Alright, yep, I'm good with this. Let's get it in and shipped. |
I've also caught not exiting behavior in image: node:10
variables:
DANGER_GITLAB_HOST: https://gitlab.com
DANGER_GITLAB_API_TOKEN: '' # placeholder, set in variables ui
test:
only:
refs:
- merge_requests
script:
- yarn install
- DEBUG="*" yarn danger ci const {message, danger} = require("danger");
const modifiedMD = danger.git.modified_files.join("- ")
message("Changed Files in this PR: \n - " + modifiedMD) DEBUG doesn't show anything extraordinary: End of stdout
|
Rebased @notjosh's work off of latest master and added support for GitLab CI as a CI source and fixed the problem with inline comments.
Resolves? #396