danger / danger-js

⚠️ Stop saying "you forgot to …" in code review
http://danger.systems/js/
MIT License
5.26k stars 368 forks source link

[BUG] addLabels not a function #1314

Closed ivankatliarchuk closed 1 year ago

ivankatliarchuk commented 2 years ago

Describe the bug

Probably not a bug, I might be missing something. According to this PR, following should just work

// Add labels
danger.gitlab.api.addLabels('Label 1', 'Label 2');

However its not a case.

To Reproduce Steps to reproduce the behavior:

const { danger } = require('danger');
danger.gitlab.api.addLabels('Label 1', 'Label 2');

TypeError: addLabels is not a function at beforeCompletion (dangerfile.js:2:9)

For ts, same result

import { danger } from "danger";
danger.gitlab.api.addLabels('Label 1', 'Label 2');

TypeError: addLabels is not a function

Expected behavior It should update labels.

Current approach

const addLabels = async (prId, input) => {
  const mrLabels = danger.gitlab.mr.labels
  const changes = [...mrLabels, ...input.filter(el => !mrLabels.includes(el, 0))];
  if (mrLabels.length != changes.length) {
    const mr = await danger.gitlab.api.MergeRequests.edit(repo, prId, { labels: [...mrLabels, ...input] })
    console.log(`updated labels '${changes}' for mr '${prId}'`)
  }
}

Labels updated.

Screenshots Screenshot 2022-09-25 at 13 21 41

Your Environment

software version
danger.js 11.1.2
node v14.16.1) and v18.9.0
npm (npm@6.14.12 and (npm v8.19.1)
Operating System Macos and Ubuntu(Docker)
orta commented 2 years ago

I don't think those are on that object (I think the PR had the wrong text) try without the .api (or check the typescript types

ivankatliarchuk commented 2 years ago

I went through the code. danger.gitlab.api actually returns InstanceType<typeof Gitlab>. Not sure if was it intentional or not, but as a result, functions addLabels and removeLabels not used anywhere in the library, would assume not in use outide of it too.

https://github.com/danger/danger-js/blob/6c49cfe3c3f8096297aa5a67e6cedf7baac16818/source/platforms/gitlab/GitLabAPI.ts#L256

Does it sound allright if I make them available through danger.gitlab.uits.addLabels ? Other option is to remove them, as its just redundant/untested piece of code.

orta commented 2 years ago

I think it is available as danger.gitlab.addLabels judging on the code

ivankatliarchuk commented 2 years ago
console.log(Object.getOwnPropertyNames(danger.gitlab))
console.log(Object.getOwnPropertyNames(danger.gitlab).filter(function (p) {
  return typeof danger.gitlab[p] === 'function';
}));

output

[ 'metadata', 'mr', 'commits', 'approvals', 'utils', 'api' ]
[]

Not the end of the world

stillborn333 commented 2 years ago

/_matrix/client/r0/auth//fallback/web?session=<session

ivankatliarchuk commented 1 year ago

Looks like there is an inconstitency in the code e.g. Github https://github.com/danger/danger-js/issues/1008

danger.github.utils.createOrAddLabel

While Gitlab does not have anything like danger.gitlab.uits.createOrAddLabel

glensc commented 1 year ago

This should add the two methods under utils:

but imho, this is a mess, why need to map, why not expose the whole "api"?

glensc commented 1 year ago

The actual breakage was from this commit by @orta:

Find source/platforms/GitLab.ts file in the diff because the commit is huge

glensc commented 1 year ago

That commit links to https://github.com/danger/danger-js/pull/1192 that has no context, at all, but 250 commits spanning multiple releases and merges or what the "improper" was. imho now the dsl exposed is improper raw GitLab class import { Gitlab } from "@gitbeaker/node" rather danger-js GitLabAPI class

glensc commented 1 year ago

Changelog is recorded in such way that the same person added addLabels/removeLabels and broke them in subsequent pr?

# 10.6.6
Fix for supporting Bitbucket Server personal repositories
GitLab: Added GitLabApi to danger.gitlab.api. - [@shyim]
GitLab: Added label helper functions to danger.gitlab.api.addLabels and danger.gitlab.api.removeLabels. - [@shyim]

cc @shyim