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

Adds support for GitLab CI #855

Merged
merged 42 commits into from
Jun 6, 2019
Merged

Adds support for GitLab CI #855

merged 42 commits into from
Jun 6, 2019

Conversation

bigkraig
Copy link
Member

@bigkraig bigkraig commented Apr 9, 2019

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

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.

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.

source/dsl/DangerDSL.ts Show resolved Hide resolved
source/platforms/gitlab/GitLabGit.ts Show resolved Hide resolved
@@ -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)
Copy link
Member

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

source/runner/Executor.ts Show resolved Hide resolved
@bigkraig
Copy link
Member Author

bigkraig commented Apr 9, 2019

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+

@orta
Copy link
Member

orta commented Apr 9, 2019

Are those tests easily scoped to just a few files which we can force to only run on Node 10?
Or does it need to be the 5.0 branch? 4.x is node 8 also?

@bigkraig
Copy link
Member Author

bigkraig commented Apr 9, 2019

@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?

@orta
Copy link
Member

orta commented Apr 9, 2019

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)

Copy link
Contributor

@notjosh notjosh left a 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! :shipit:

.vscode/launch.json Outdated Show resolved Hide resolved
@bigkraig
Copy link
Member Author

bigkraig commented Apr 9, 2019

Before I go and spend more time trying to chase down why my fileContents function isn't working, is this the right way to go about using it?

import {DangerDSLType} from "./source/dsl/DangerDSL"
declare var danger: DangerDSLType

const rsFiles = danger.git.modified_files.filter(a => a.endsWith(".rs"))
rsFiles.forEach(function (filename) {
  let cnt = danger.gitlab.utils.fileContents(filename)
  console.log(cnt)
})

I get that it's not a function (code is not committed yet but i'm setting it everywhere AFAIK.

danger.gitlab.utils.fileContents is not a function
TypeError: danger.gitlab.utils.fileContents is not a function
    at dangerfile.fileContents.ts:5:35

@orta
Copy link
Member

orta commented Apr 9, 2019

Close, it should be a promise: let cnt = await danger.gitlab.utils.fileContents(filename)

@notjosh
Copy link
Contributor

notjosh commented Apr 9, 2019

Note that I didn't implement the utils yet (mentioned at the tail end of here fwiw), so the functions don't exit: https://github.com/bigkraig/danger-js/blob/f0d1e81e90060bab752bae15a9c6698e2cfabc48/source/platforms/GitLab.ts#L36

Jamie Thompson and others added 3 commits May 29, 2019 16:02
- 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
@f-meloni
Copy link
Member

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
@jamime
Copy link
Contributor

jamime commented May 30, 2019

Latest version available using @jamime/danger@next or @jamime/danger:7.1.5-next.9

@orta
Copy link
Member

orta commented Jun 3, 2019

Heyyyyy, this is green!

@bigkraig @jamime - I'm happy to get this in and shipped when you are?

@jamime
Copy link
Contributor

jamime commented Jun 3, 2019 via email

Jamie Thompson added 5 commits June 4, 2019 14:43
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
@jamime
Copy link
Contributor

jamime commented Jun 5, 2019

@orta yarn test passes for me on node 12.4.0 but fails in Travis. Any ideas? Feature wise we're pretty much there now, just getStructuredDiffForFile yet to implement which will fix the remaining features in the Git DSL.

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.

@orta
Copy link
Member

orta commented Jun 5, 2019

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

@jamime
Copy link
Contributor

jamime commented Jun 5, 2019

@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:

  • danger.git.diffForFile
  • danger.git.structuredDiffForFile
  • danger.git.linesOfCode

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.

@orta
Copy link
Member

orta commented Jun 5, 2019

I'd try running danger-pr with DEBUG="*" and see what the last log is?

Re: CI, I was wondering if somehow on that specific instance of CI TypeScript gets updated

@jamime
Copy link
Contributor

jamime commented Jun 6, 2019

@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.

  danger:executor Writing to STDOUT: {
  fails: [],
  warnings: [
    {
      message: 'Please add a changelog entry for your changes. You can find it in ' +
        '`CHANGELOG.md` \n\nPlease add your change and name to the master ' +
        'section.'
    },
    {
      message: 'These providers are missing from the README: GitHub Actions'
    }
  ],
  messages: [],
  markdowns: [],
  scheduled: []
} +0ms
Danger: ✓ found only warnings, not failing the build
## Warnings
Please add a changelog entry for your changes. You can find it in `CHANGELOG.md` 

Please add your change and name to the master section.
-
These providers are missing from the README: GitHub Actions

✔️ Travis is fixed.

@jamime
Copy link
Contributor

jamime commented Jun 6, 2019

@orta can you clear the cache on Travis and click off the build again please.

@DangerCI
Copy link

DangerCI commented Jun 6, 2019

Warnings
⚠️ These providers are missing from the README: GitHub Actions

nock

Author: Pedro Teixeira

Description: HTTP server mocking and expectations library for Node.js

Homepage: https://github.com/nock/nock#readme

Createdover 7 years ago
Last Updated9 days ago
LicenseMIT
Maintainers3
Releases326
Direct Dependencieschai, debug, deep-equal, json-stringify-safe, lodash, mkdirp, propagate, qs and semver

@types/nock

Author: Unknown

Description: TypeScript definitions for nock

Homepage: http://npmjs.com/package/@types/nock

Createdabout 3 years ago
Last Updated8 days ago
LicenseMIT
Maintainers1
Releases27
Direct Dependencies@types/node
README

Installation

npm install --save @types/nock

Summary

This package contains type definitions for nock ( https://github.com/nock/nock ).

Details

Files were exported from https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/nock

Additional Details

  • Last updated: Tue, 14 May 2019 20:53:20 GMT
  • Dependencies: @types/node
  • Global values: none

Credits

These definitions were written by bonnici https://github.com/bonnici, Horiuchi_H https://github.com/horiuchi, afharo https://github.com/afharo, Matt R. Wilson https://github.com/mastermatt, Garanzha Dmitriy https://github.com/damour, GP https://github.com/paambaati.

gitlab

Author: Justin Dalrymple

Description: Full NodeJS implementation of the GitLab API. Supports Promises, Async/Await.

Homepage: https://github.com/jdalrymple/node-gitlab#readme

Createdover 6 years ago
Last Updated4 days ago
LicenseMIT
Maintainers3
Releases110
Direct Dependenciesform-data, humps, ky, ky-universal, li, query-string, randomstring and universal-url
Keywordsapi, browser, es5, es6, gitlab and ky
This README is too long to show.

New dependencies added: gitlab, @types/nock and nock.

Generated by 🚫 dangerJS against b386dae

@orta
Copy link
Member

orta commented Jun 6, 2019

Alright, yep, I'm good with this. Let's get it in and shipped.

@orta orta merged commit 583eae5 into danger:master Jun 6, 2019
@peril-staging
Copy link
Contributor

peril-staging bot commented Jun 6, 2019

Thanks for the PR @bigkraig.

This PR has been shipped in v8.0.0 - CHANGELOG.

@ewnd9
Copy link

ewnd9 commented Jul 1, 2019

I've also caught not exiting behavior in v9.0.2, how can I help to debug it?

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
2019-07-01T21:58:57.530Z danger:GitLabAPI updateMergeRequestNote { id: 187252374,
  type: null,
  body:
   '\n<!--\n  0 failure: \n  0 warning: \n  1 messages\n  \n  DangerID: danger-id-Danger;\n-->\n\n\n\n<table>\n  <thead>\n    <tr>\n      <th width="50"></th>\n      <th width="100%" data-danger-table="true">Messages</th>\n    </tr>\n  </thead>\n  <tbody><tr>\n      <td>:book:</td>\n      <td>Changed Files in this PR: \n - </td>\n    </tr>\n  </tbody>\n</table>\n\n\n<p align="right">\n  Generated by :no_entry_sign: <a href="https://danger.systems/js">dangerJS</a> against 19b492e2e325ca6ded103cfaac45567202e71582\n</p>\n',
  attachment: null,
  author:
   { id: 382868,
     name: 'ewnd9',
     username: 'ewnd9',
     state: 'active',
     avatar_url:
      'https://assets.gitlab-static.net/uploads/-/system/user/avatar/382868/avatar.png',
     web_url: 'https://gitlab.com/ewnd9' },
  created_at: '2019-07-01T21:23:26.115Z',
  updated_at: '2019-07-01T21:58:57.357Z',
  system: false,
  noteable_id: 32503584,
  noteable_type: 'MergeRequest',
  resolvable: false,
  noteable_iid: 1 }
Feedback: https://gitlab.com/ewnd9/danger-playground/merge_requests/1#note_187252374
2019-07-01T21:58:57.531Z danger:GitLab updateStatus {}
Done in 303.05s.

@jamime
Copy link
Contributor

jamime commented Jul 1, 2019

@ewnd9 issue reported here #875

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.

None yet

8 participants