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

feat: add support for setting assignees/reviewers from code owners #6244

Merged
merged 25 commits into from
May 30, 2020
Merged

feat: add support for setting assignees/reviewers from code owners #6244

merged 25 commits into from
May 30, 2020

Conversation

fgreinacher
Copy link
Contributor

Closes #6016

@fgreinacher
Copy link
Contributor Author

fgreinacher commented May 14, 2020

Missing things:

  • Declare config option
  • Documentation
  • Same feature for reviewers?

lib/workers/pr/index.ts Outdated Show resolved Hide resolved
@@ -161,6 +162,15 @@ export interface RenovateConfig
regexManagers?: CustomManager[];
}

export interface AssigneesAndReviewersConfig {
assigneesFromCodeowners?: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure whether I like this name. Alternative ideas?

lib/config/common.ts Outdated Show resolved Hide resolved
lib/workers/pr/index.ts Outdated Show resolved Hide resolved
lib/workers/pr/index.ts Outdated Show resolved Hide resolved
lib/workers/pr/index.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@fgreinacher fgreinacher marked this pull request as ready for review May 15, 2020 22:12
@fgreinacher fgreinacher requested a review from viceice May 15, 2020 22:12
@fgreinacher fgreinacher changed the title feat: add support for setting assignees from code owners feat: add support for setting assignees/reviewers from code owners May 15, 2020
lib/workers/pr/code-owners.spec.ts Outdated Show resolved Hide resolved
lib/workers/pr/code-owners.spec.ts Outdated Show resolved Hide resolved
lib/workers/pr/code-owners.ts Outdated Show resolved Hide resolved
fgreinacher and others added 4 commits May 16, 2020 13:15
Co-authored-by: Michael Kriese <michael.kriese@visualon.de>
Co-authored-by: Michael Kriese <michael.kriese@visualon.de>
Co-authored-by: Michael Kriese <michael.kriese@visualon.de>
@fgreinacher fgreinacher requested a review from viceice May 16, 2020 11:46
viceice
viceice previously approved these changes May 16, 2020
@fgreinacher
Copy link
Contributor Author

Pulled changes from master.

Error in Windows run seems unrelated to my changes?!:


16
yarn run v1.22.4
17
$ cross-env NODE_ENV=test LOG_LEVEL=fatal node --expose-gc node_modules/jest/bin/jest.js --maxWorkers=2 --ci --coverage false
18
Error: Cannot find module 'jest-serializer'
19
Require stack:
20
- D:\a\renovate\renovate\node_modules\jest-haste-map\build\index.js
21
- D:\a\renovate\renovate\node_modules\jest-runtime\build\index.js
22
- D:\a\renovate\renovate\node_modules\@jest\core\build\cli\index.js
23
- D:\a\renovate\renovate\node_modules\@jest\core\build\jest.js
24
- D:\a\renovate\renovate\node_modules\jest-cli\build\cli\index.js
25
- D:\a\renovate\renovate\node_modules\jest-cli\bin\jest.js
26
- D:\a\renovate\renovate\node_modules\jest\bin\jest.js
27
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:957:15)
28
    at Function.Module._load (internal/modules/cjs/loader.js:840:27)
29
    at Module.require (internal/modules/cjs/loader.js:1019:19)
30
    at require (internal/modules/cjs/helpers.js:77:18)
31
    at _jestSerializer (D:\a\renovate\renovate\node_modules\jest-haste-map\build\index.js:64:39)
32
    at HasteMap._persist (D:\a\renovate\renovate\node_modules\jest-haste-map\build\index.js:786:5)
33
    at D:\a\renovate\renovate\node_modules\jest-haste-map\build\index.js:400:16
34
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
35
    at async D:\a\renovate\renovate\node_modules\@jest\core\build\cli\index.js:289:9
36
    at async Promise.all (index 0)
37
error Command failed with exit code 1.
38
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@fgreinacher
Copy link
Contributor Author

Erm, 082cd4e#diff-5a082d2a82fa7048de40c1d26bf82af0 removed Platform.getPrFiles which I used here 😕

@fgreinacher
Copy link
Contributor Author

fgreinacher commented May 24, 2020

@rarkins How should we proceed here? Add back the missing function or drop this approach?

@rarkins
Copy link
Collaborator

rarkins commented May 24, 2020

@fgreinacher sorry for breaking your PR!

Just now I was thinking that in theory we should be able to know which files are changed in our own PRs without asking the platform. However then I realized that we have scenarios where the branch (i.e. commit) is done in one run and the PR created in another. We could probably be able to determine from config which package files are changed, but not which lock files.

Another platform-independent approach (and hence much less maintenance than the previous getPrFiles) would be to use git to determine which files were changed in the last commit of the branch. Do you think that would be feasible? I can try helping advise on that if you like the idea.

@fgreinacher
Copy link
Contributor Author

@fgreinacher sorry for breaking your PR!

No worries!

We could probably be able to determine from config which package files are changed, but not which lock files.

True, also people might have post-upgrade commands that modify things.

Another platform-independent approach (and hence much less maintenance than the previous getPrFiles) would be to use git to determine which files were changed in the last commit of the branch. Do you think that would be feasible? I can try helping advise on that if you like the idea.

Sounds reasonable, looking on this now.

@fgreinacher fgreinacher reopened this May 25, 2020
@fgreinacher
Copy link
Contributor Author

fgreinacher commented May 25, 2020

Sounds reasonable, looking on this now.

@rarkins What do yo think about adding back getPrFiles to the platform API and implementing it through platform/git?

Edit: See 881a19a

@rarkins
Copy link
Collaborator

rarkins commented May 26, 2020

@fgreinacher I think it looks good. Maybe reduce duplication by moving the return diff.files.map((x) => x.file); into git too?

I had also wondered if we should make it getBranchFiles() in case we ever want the list of files of a branch without PR?

@fgreinacher
Copy link
Contributor Author

fgreinacher commented May 26, 2020

I had also wondered if we should make it getBranchFiles() in case we ever want the list of files of a branch without PR?

Yeah, that sounds reasonable. 647610f moves the core getBranchFiles logic to storage. At the platform level I left it as getPrFiles because I considered the mapping pr->branch a platform concern.

I did not find a cool way to get rid of the duplication across platforms, but it's just a one-liner, so OK IMO. I also excluded those one-liners from coverage (similar to some other functions that mainly forward to storage).

package.json Outdated Show resolved Hide resolved
@rarkins rarkins merged commit 81a5dee into renovatebot:master May 30, 2020
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 20.13.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@fgreinacher fgreinacher deleted the feat/assignees-from-codeowners branch June 14, 2020 13:01
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Determine assignees from CODEOWNERS
4 participants