GitLab FOSS issueshttps://gitlab.com/gitlab-org/gitlab-foss/-/issues2019-09-06T08:46:40Zhttps://gitlab.com/gitlab-org/gitlab-foss/-/issues/67100Danger comment is missing2019-09-06T08:46:40ZLin Jen-ShinDanger comment is missingIt's broken by https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/32196
Sorry for not checking this before merging :(
I tried to rebase on https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/15930 and now danger didn't give anyt...It's broken by https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/32196
Sorry for not checking this before merging :(
I tried to rebase on https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/15930 and now danger didn't give anything: https://gitlab.com/gitlab-org/gitlab-ee/-/jobs/289141018
It was giving https://gitlab.com/gitlab-org/gitlab-ee/-/jobs/28851655812.3Ash McKenzieamckenzie@gitlab.comAsh McKenzieamckenzie@gitlab.comhttps://gitlab.com/gitlab-org/gitlab-foss/-/issues/66596Allow Danger to be run locally - initial work2019-09-13T05:31:28ZAsh McKenzieamckenzie@gitlab.comAllow Danger to be run locally - initial workIt would be awesome if we could run as many of the Danger rules locally as we could not only save CI jobs but also reduce the feedback loop by being able execute fast running Danger rules before needing to push.It would be awesome if we could run as many of the Danger rules locally as we could not only save CI jobs but also reduce the feedback loop by being able execute fast running Danger rules before needing to push.12.3Ash McKenzieamckenzie@gitlab.comAsh McKenzieamckenzie@gitlab.comhttps://gitlab.com/gitlab-org/gitlab-foss/-/issues/65317Figure out why Danger didn't pick a Test Automation Engineer2019-07-31T11:32:17ZLin Jen-ShinFigure out why Danger didn't pick a Test Automation EngineerIn https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/31137 :
![Screen_Shot_2019-07-30_at_19.06.13](/uploads/ab99d3c5b2e8dd45da4e83281e59acf3/Screen_Shot_2019-07-30_at_19.06.13.png)
A quick check seems the code should be fine, but ...In https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/31137 :
![Screen_Shot_2019-07-30_at_19.06.13](/uploads/ab99d3c5b2e8dd45da4e83281e59acf3/Screen_Shot_2019-07-30_at_19.06.13.png)
A quick check seems the code should be fine, but something might be changed in https://about.gitlab.com/roulette.json12.2Lin Jen-ShinLin Jen-Shinhttps://gitlab.com/gitlab-org/gitlab-foss/-/issues/65312Use stage labels instead of legacy team labels for Danger picking Test reviewers2019-08-27T13:00:31ZLin Jen-ShinUse stage labels instead of legacy team labels for Danger picking Test reviewersIn https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/28678 we added Test Automation Engineers to the Danger roulette to review tests based on the legacy team labels.
When we fully move to group labels, we need to update respectively.In https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/28678 we added Test Automation Engineers to the Danger roulette to review tests based on the legacy team labels.
When we fully move to group labels, we need to update respectively.12.3Rémy CoutableRémy Coutablehttps://gitlab.com/gitlab-org/gitlab-foss/-/issues/62015Add counterpart TAE to the reviewer roulette2019-07-09T20:17:09ZMek StittriAdd counterpart TAE to the reviewer rouletteAn effective iteration to asking Test Automation Engineers to review tests under `spec/features` is to list them as part of the reviewer.
We could add a category for ~test under the same table below.
| Category | Reviewer | Maintainer...An effective iteration to asking Test Automation Engineers to review tests under `spec/features` is to list them as part of the reviewer.
We could add a category for ~test under the same table below.
| Category | Reviewer | Maintainer |
| -------- | -------- | ---------- |
| ~frontend | [Dennis Tang](https://gitlab.com/dennis) (`@dennis`) | [Phil Hughes](https://gitlab.com/iamphill) (`@iamphill`) |
| ~test | [Dan Davison](https://gitlab.com/ddavison) (`@ddavison`) | |
![Screen_Shot_2019-05-19_at_3.06.04_PM](/uploads/6aff3bc9485e4d30b72681d79b1a4dc7/Screen_Shot_2019-05-19_at_3.06.04_PM.png)12.0Lin Jen-ShinLin Jen-Shinhttps://gitlab.com/gitlab-org/gitlab-foss/-/issues/61157Reviewer roulette shouldn't include the author as a possibility2019-08-07T00:01:27ZRobert SpeicherReviewer roulette shouldn't include the author as a possibilityThis happened here: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/27855This happened here: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/2785512.0🙈 jacopo beschi 🙉intrip@gmail.com🙈 jacopo beschi 🙉intrip@gmail.comhttps://gitlab.com/gitlab-org/gitlab-foss/-/issues/61047Docs: danger bot - reviewer roulette2019-04-30T10:38:54ZMarcia RamosDocs: danger bot - reviewer roulette### Problem to solve
Currently, we have two conflicting review guidelines:
- Reviewer by product category
- Reviewer roullete
They're conflicting; we need to decide whether we want to choose the reviewer by the category or randomly by...### Problem to solve
Currently, we have two conflicting review guidelines:
- Reviewer by product category
- Reviewer roullete
They're conflicting; we need to decide whether we want to choose the reviewer by the category or randomly by the roullete. Having both options is confusing for people assigning MRs to us.
### Proposal
To make things easier, add the information about who's reviewing what on the [docs development guidelines](https://docs.gitlab.com/ee/development/documentation/):
>>>
Each technical writer is assigned individually to review MRs according to the product categories:
|Axil|Evan|Marcia|Matt|Mike|Russell|
|--|--|--|--|--|--|
xxx | xxx | xxx | xxx | xxx |xxx|
>>>
The [handbook link](https://about.gitlab.com/handbook/product/categories/) does not include:
- ~"development guidelines"
- ~markdown
- ~"description templates"
We'd leave the info in https://about.gitlab.com/handbook/product/categories/ for reference, but we'd repeat it in the dev guidelines to include the missing things.
Alternatively, we can use the reviewer roullete and don't split us into product-focused areas.
### Who can address the issue
@gl\-docsteam
### Other links/references
<!-- E.g. related GitLab issues/MRs -->11.11Evan ReadEvan Readhttps://gitlab.com/gitlab-org/gitlab-foss/-/issues/60475Add rule to Danger to detect possible CE/EE Vue <template> issues2019-04-24T14:37:00ZNathan Friendhello@nathanfriend.ioAdd rule to Danger to detect possible CE/EE Vue <template> issues### Problem to solve
If a developer makes a change to the `<template>` of a CE Vue file (e.g. `path/to/file.vue`), and that file has a EE counterpart in the EE repo (e.g. `ee/path/to/file.vue`), the developer will need to make their cha...### Problem to solve
If a developer makes a change to the `<template>` of a CE Vue file (e.g. `path/to/file.vue`), and that file has a EE counterpart in the EE repo (e.g. `ee/path/to/file.vue`), the developer will need to make their change in **both** places. If they forget, their changes won't be visible in EE, since the template of the EE Vue file will take precedence, blowing away the CE version of the template.
This is an especially easy mistake to make since the EE file isn't in the CE repo, where the developer is working - it's in an entirely different repo.
### Intended users
<!-- Who will use this feature? If known, include any of the following: types of users (e.g. Developer), personas, or specific company roles (e.g. Release Manager). It's okay to write "Unknown" and fill this field in later.
Personas can be found at https://about.gitlab.com/handbook/marketing/product-marketing/roles-personas/ -->
~"Persona: Software developer"
### Further details
Here's an example of this happening: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/25983#note_160056412
This problem should go away once we combine CE and EE into a single codebase.
### Proposal
Add a warning to Danger to notify the developer when they make a change to a CE Vue file that has a corresponding Vue file in the EE repo (or vice-versa).
Something like:
`You made a change to path/to/file.vue. An EE version of this file exists in the EE repo at ee/path/to/file.vue. Did you ensure any template changes have been made in both places?`Nathan Friendhello@nathanfriend.ioNathan Friendhello@nathanfriend.iohttps://gitlab.com/gitlab-org/gitlab-foss/-/issues/60035Weight trainee maintainers higher in reviewer roulette2022-04-25T18:16:47ZSean McGivernWeight trainee maintainers higher in reviewer rouletteThe people in https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_trainee-maintainers should get picked more often by reviewer roulette so they get more practice reviewing.
Naively, for backend:
1. Currently each trainee ...The people in https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_trainee-maintainers should get picked more often by reviewer roulette so they get more practice reviewing.
Naively, for backend:
1. Currently each trainee maintainer has a 1/27 chance of being picked as reviewer.
2. If we added them all twice in the list (like `[@ashmckenzie, @ashmckenzie, ...]`), that would be 2/33.
3. Three times would be 3/39, which is more than twice as likely than it is currently!
@gitlab\-org/maintainers/rails\-backend what do you think?11.11Sean McGivernSean McGivernhttps://gitlab.com/gitlab-org/gitlab-foss/-/issues/58231Documentation Danger message is too verbose2019-03-13T09:49:31ZSean McGivernDocumentation Danger message is too verbosehttps://gitlab.com/gitlab-org/gitlab-ee/merge_requests/9719#note_145208466 isn't a great message:
1. I already assigned the right person.
2. The message takes more than a page on my screen (using an external monitor).
Some of this is f...https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/9719#note_145208466 isn't a great message:
1. I already assigned the right person.
2. The message takes more than a page on my screen (using an external monitor).
Some of this is from the reviewer roulette message, but it's mostly from the documentation message, which is far too verbose. I'd go as far as saying that it's so verbose it's making me less likely to read Danger comments in future, because this one was so long, and yet completely unnecessary.
I think we should change this so that most of it links to a SSOT, rather than trying to include all the information someone might need in the comment itself. Most people seeing this are GitLabbers, because Danger doesn't run on forks, and they will see this a lot. They don't need this explained from first principles each time :slight\_smile:Mike LewisMike Lewishttps://gitlab.com/gitlab-org/gitlab-foss/-/issues/58192Documentation about adding to danger2019-09-03T00:23:56ZNick ThomasDocumentation about adding to dangerReviewer roulette was implemented in danger, and while doing so, I ran into a bunch of challenges. I should improve the docs to help people who come after!Reviewer roulette was implemented in danger, and while doing so, I ran into a bunch of challenges. I should improve the docs to help people who come after!Nick ThomasNick Thomashttps://gitlab.com/gitlab-org/gitlab-foss/-/issues/57912Ignore `Revert` commits for Danger2019-02-20T15:52:53ZKamil TrzcińskiIgnore `Revert` commits for DangerWe had today a fatal danger failure due to `Revert ""` commit be present on branch: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/25393#note_142741646
We should rather ignore commit message offenses for `Revert` commits.We had today a fatal danger failure due to `Revert ""` commit be present on branch: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/25393#note_142741646
We should rather ignore commit message offenses for `Revert` commits.Kamil TrzcińskiKamil Trzcińskihttps://gitlab.com/gitlab-org/gitlab-foss/-/issues/57766Reviewer roulette should give stable recommentations2019-04-16T11:48:32ZNick ThomasReviewer roulette should give stable recommentationsCurrently, reviewer roulette runs every time someone pushes to their merge request. Each time it runs, it makes a random selection of reviewer and maintainer. This means the selections are **not stable** - they change every time a push h...Currently, reviewer roulette runs every time someone pushes to their merge request. Each time it runs, it makes a random selection of reviewer and maintainer. This means the selections are **not stable** - they change every time a push happens.
In the introducing MR, we discussed this a little, and I wasn't sure whether it would be annoying or not, so we left it at the simplest iteration.
Having experienced it for a couple of days, it is starting to annoy me, so perhaps it should change?
Implementation-wise, we spoke of seeding the random number generator to get a stable recommendation, but that has a number of limitations, and doesn't guarantee stability in some circumstances.
I wonder if we could read the recommendations made in the last run, if we mark up the table we're outputting to the GitLab comment appropriately. We can then re-use any suggestions that already exist, while adding new suggestions if the list of categories change and some are unfilfilled.
cc @DouweM @godfatSean McGivernSean McGivernhttps://gitlab.com/gitlab-org/gitlab-foss/-/issues/57680Reviewer roulette should ignore some files2019-08-07T07:59:51ZNick ThomasReviewer roulette should ignore some filesParticularly, files in `changelog/` do not need to be reviewed, and so shouldn't be included in any reviewed category *or* the `unknown` category.Particularly, files in `changelog/` do not need to be reviewed, and so shouldn't be included in any reviewed category *or* the `unknown` category.11.9Nick ThomasNick Thomashttps://gitlab.com/gitlab-org/gitlab-foss/-/issues/57652Reviewer roulette via Danger should take OOO status into account2019-04-30T08:50:57ZNick ThomasReviewer roulette via Danger should take OOO status into accountThe following discussion from !24938 should be addressed:
- [ ] @rymai started a [discussion](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/24938#note_141032433):
> **question:** Do we have an issue for that? If not we sh...The following discussion from !24938 should be addressed:
- [ ] @rymai started a [discussion](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/24938#note_141032433):
> **question:** Do we have an issue for that? If not we should probably create a follow-up issue.
We're introducing reviewer roulette - merge requests will have possible reviewers added as suggestions. However, we don't currently take out-of-office status into account, and it would be nice to do so.
GitLab recently stated handling PTO with PTO ninja, so we could consider a direct integration there. Other options include google calendars, the GitLab "status" field itself, etc.
Enhancements could include the ability to take yourself out of reviews without being OOO if, say, you've got too many.
cc @DouweM @grzesiekhttps://gitlab.com/gitlab-org/gitlab-foss/-/issues/55709Danger GitLab reference check should be more precise2019-01-08T18:00:43ZLin Jen-ShinDanger GitLab reference check should be more preciseThe following discussion from gitlab-ee!8960 should be addressed:
- [ ] @godfat started a [discussion](https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/8960#note_127132259):
> Danger says ``Use full URLs instead of short ref...The following discussion from gitlab-ee!8960 should be addressed:
- [ ] @godfat started a [discussion](https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/8960#note_127132259):
> Danger says ``Use full URLs instead of short references (`gitlab-org/gitlab-ce#123` or `!123`), as short references are displayed as plain text outside of GitLab`` but this doesn't make sense. The message was:
>
> ```
> Properly override sign_in_and_redirect with right arity
>
> for `Devise::Controllers::Helpers#sign_in_and_redirect`,
> which has arity of -2:
> https://www.rubydoc.info/github/plataformatec/devise/Devise%2FControllers%2FHelpers:sign_in_and_redirect
> ```
>
> There's no reference to any issue.11.8Rémy CoutableRémy Coutablehttps://gitlab.com/gitlab-org/gitlab-foss/-/issues/55142Danger `too_many_changed_lines?` check should ignore files in `vendor/`2019-12-02T11:55:57ZRobert SpeicherDanger `too_many_changed_lines?` check should ignore files in `vendor/`I don't think MRs like https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/23706 should need to explain the changes in `vendor/` in detail.
cc @yorickpeterseI don't think MRs like https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/23706 should need to explain the changes in `vendor/` in detail.
cc @yorickpetersehttps://gitlab.com/gitlab-org/gitlab-foss/-/issues/54490Detection of /doc-only changes in non-docs branch, and vice versa2019-09-10T23:50:57ZMike LewisDetection of /doc-only changes in non-docs branch, and vice versa- What happens when someone commits a non-`/doc` code change to a docs branch? Is there something in the current default CE or EE pipeline that fails? Is it clear why it fails?
- When someone commits a `/doc`-only change but fails to us...- What happens when someone commits a non-`/doc` code change to a docs branch? Is there something in the current default CE or EE pipeline that fails? Is it clear why it fails?
- When someone commits a `/doc`-only change but fails to use a docs branch, should we have DangerBot remind them? Something like this:
> For future reference, when only making changes to the /doc path, [using the special docs branch names](https://docs.gitlab.com/ee/development/documentation/#branch-naming) will skip the unnecessary pipeline steps required for other code. However, you can still merge this branch if the current (extended) pipeline passes.Lukas 'ai-pi' Eipertleipert@gitlab.comLukas 'ai-pi' Eipertleipert@gitlab.com