renovatebot / renovate

Home of the Renovate CLI: Cross-platform Dependency Automation by Mend.io
https://mend.io/renovate
GNU Affero General Public License v3.0
17.17k stars 2.24k forks source link

Detect and error if no write permissions #4223

Open RmStorm opened 5 years ago

RmStorm commented 5 years ago

What would you like Renovate to be able to do? Improved error handling/messaging on branch/PR creation.

Describe the solution you'd like I would like these two lines to give a better description of what is going wrong when something goes wrong. repository-changed is not so clear especially when the issue is actually a permission issue and the repository didn't change at all. https://github.com/renovatebot/renovate/blob/0f19abd3443e3732ee02fa1659589b77660ce609/lib/platform/git/storage.ts#L429-L430 I'm not sure what would be a better way to do this. Maybe the function commitFilesToBranch can just have one or two extra debugging statements? Or it can throw a different error than repository-changed, something like commit-to-branch-failed.

Describe alternatives you've considered Make this PR to simple-git and ask them to throw a clearer error.

Additional context I setup a self hosted version of renovatebot by following the instructions here. Because I wanted to be careful with permissions I gave the bot read access to the repositories and thought would be enough. I though so because the read only permissions explicitly includes the ability to make PR's.

However since renovatebot does not only make PR's but also the branches from which to make the PR's it obviously requires write access. So that was a silly mistake.

The consequence from the mistake was quite confusing. Instead of some clear error I got: INFO: Repository has changed during renovation - aborting Than I enabled debuging and I got an error from simple-git:

"err": {
"message": "remote: Repository not found.\nfatal: repository 'https://**redacted**@github.com/MyOrg/my_repo.git/' not found\n",
"stack": "Error: remote: Repository not found.\nfatal: repository 'https://**redacted**@github.com/MyOrg/my_repo.git/' not found\n\n at /usr/src/app/node_modules/simple-git/promise.js:62:29\n at Git.<anonymous> (/usr/src/app/node_modules/simple-git/src/git.js:954:21)\n at Function.Git.fail (/usr/src/app/node_modules/simple-git/src/git.js:1488:18)\n at fail (/usr/src/app/node_modules/simple-git/src/git.js:1446:20)\n at /usr/src/app/node_modules/simple-git/src/git.js:1455:16\n at process._tickCallback (internal/process/next_tick.js:68:7)"
}

Apparently Git.<anonymous> was causing my problem. That didn't help a lot.

I think it would also be nice to clarify in the self-hosting readme that the user account needs write access.

Another solution would be to have the branches created on a forked repository stored in the user account used by renovatebot. However that is a large architectural change to renovatebot which I don't think will be remotely feasible.

RmStorm commented 5 years ago

I think there is some room for improvement here but maybe I should just get better at understanding error messages. Feel free to close this issue if you think the error handling is currently adequate.

elwynelwyn commented 5 years ago

I just went down this rabbit hole, thought the repository has changed error was related to lint-staged..

I just hadn't granted enough permissions for renovate to create a new branch on the repo.

rarkins commented 5 years ago

I think this can be summarized as: if Renovate lacks sufficient privilege to write to the repo, we should report that as the error instead of "repository changed".

Next step is to work out how we can detect this. In @RmStorm's case the error is quite perplexing: remote: Repository not found does not sound like your typical "you can read but not write" error. @elwynelwyn what stack trace were you getting?

elwynelwyn commented 5 years ago

So I'm running Renovate via this AzureDevOps task: https://marketplace.visualstudio.com/items?itemName=jyc.vsts-extensions-renovate-me

These are all the logs I'm able to see (I think?):

home/vsts/.yarn/bin/renovate MyOrg/MyRepo --platform azure --endpoint https://myorg.visualstudio.com/ --token ***
 INFO: Using default gitAuthor: Renovate Bot <bot@renovateapp.com>
 INFO: Renovating repository (repository=MyOrg/MyRepo)
 INFO: Initialising git repository into /tmp/renovate/repos/azure/MyOrg/MyRepo (repository=MyOrg/MyRepo)
 INFO: git clone completed (repository=MyOrg/MyRepo)
       "cloneSeconds": 1.4
 INFO: Setting git author (repository=MyOrg/MyRepo)
       "gitAuthor": {"name": "Renovate Bot", "email": "bot@renovateapp.com"}
 INFO: Repo is not onboarded (repository=MyOrg/MyRepo)
 INFO: Found npm package files (repository=MyOrg/MyRepo)
 INFO: Found nuget package files (repository=MyOrg/MyRepo)
 INFO: Need to create onboarding PR (repository=MyOrg/MyRepo)
 INFO: Creating onboarding branch (repository=MyOrg/MyRepo)
 INFO: Repository has changed during renovation - aborting (repository=MyOrg/MyRepo)
 INFO: Finished repository (repository=MyOrg/MyRepo)
rarkins commented 5 years ago

Can you set LOG_LEVEL=debug in env?

elwynelwyn commented 5 years ago

debug-log.txt

See attached

rarkins commented 5 years ago
2019-09-02T23:16:17.2797928Z DEBUG: 2 file(s) to commit (repository=MyOrg/MyRepo, dependencies=@types/node, branch=renovate/npm/node-12.x)
2019-09-02T23:16:17.2799568Z DEBUG: Committing files to branch renovate/npm/node-12.x (repository=MyOrg/MyRepo, dependencies=@types/node, branch=renovate/npm/node-12.x)
2019-09-02T23:16:17.8553423Z DEBUG: Error commiting files (repository=MyOrg/MyRepo, dependencies=@types/node, branch=renovate/npm/node-12.x)
2019-09-02T23:16:17.8554285Z        "err": {
2019-09-02T23:16:17.8555196Z          "message": "To https://**redacted**@myorg.visualstudio.com/MyOrg/_git/MyRepo'\n",
2019-09-02T23:16:17.8555937Z          "stack": "Error: To https://**redacted**@myorg.visualstudio.com/MyOrg/_git/MyRepo'\n\n    at /home/vsts/.config/yarn/global/node_modules/simple-git/promise.js:62:29\n    at Git.<anonymous> (/home/vsts/.config/yarn/global/node_modules/simple-git/src/git.js:954:21)\n    at Function.Git.fail (/home/vsts/.config/yarn/global/node_modules/simple-git/src/git.js:1488:18)\n    at fail (/home/vsts/.config/yarn/global/node_modules/simple-git/src/git.js:1446:20)\n    at /home/vsts/.config/yarn/global/node_modules/simple-git/src/git.js:1455:16\n    at process._tickCallback (internal/process/next_tick.js:68:7)"
2019-09-02T23:16:17.8556036Z        }
rarkins commented 5 years ago

The git layer can only really test write access by attempting to push a commit to the server. Unfortunately in the above the message is not clearly a permissions one, so the best we could do is a message like "Are you sure you have write permissions?".

Perhaps the better solution is to test write permissions during initRepo (which would vary per-platform).

elwynelwyn commented 5 years ago

Are you sure you have write permissions?

That would have helped me out a lot! Even surfacing the Error commiting files message would be useful.

rarkins commented 5 years ago

Summary of action to take:

souravdasslg commented 4 years ago

Thinking of breaking this issue into multiple PR. Possible solutions for GitHub:

https://github.com/renovatebot/renovate/blob/b93abef11a2af3b38dda1a9c07a26a4d47ac70d5/lib/platform/github/index.ts#L235-L241

Adding a check here if permission.push is false by checking this object

"permissions": {
    "admin": false,
    "push": false,
    "pull": true
  }
rarkins commented 4 years ago

@souravdasslg beware that GitHub had and probably still has a bug where apps (eg the gusted WhiteSource Renovate) report no push permissions even though they have them. Ie this feature was not reliable when I attempted to use it last year.

jamesharv commented 4 years ago

I ran into this error using the Github App, but in my case it was because I had previously created a branch in my repo called renovate.

It seems that GitHub won't allow namespaced branches like renovate/package-name1.x to be created when a branch with the namespace name (ie renovate) already exists.

Deleting the renovate branch resolved it for me. Hope that helps someone.

pret-a-porter commented 3 years ago

@rarkins I will take this one

pret-a-porter commented 3 years ago

Is it the same as #9593 ?

viceice commented 3 years ago

Is it the same as #9593 ?

Seems so to me. 🤷‍♂️

rarkins commented 3 years ago

There's also an additional issue saying that a token shouldn't be necessary for a dry run. Can you combine it?

pret-a-porter commented 3 years ago

@rarkins do you mean for Azure platform?

rarkins commented 3 years ago

No, on general. Renovate quits of no token is specified but when doing a dry run that shouldn't be necessary

pret-a-porter commented 3 years ago

@rarkins that is what I found for github https://docs.github.com/en/rest/reference/repos#collaborators, I don't see way to check write permissions without token. Correct me, if I am wrong.

rarkins commented 3 years ago

That's correct. You can't write without a token. But I think you are misunderstanding.

pret-a-porter commented 3 years ago

Ah, now I got it. But it looks like separate issue, because it throws an error without token even in dry-run mode for now.

rarkins commented 3 years ago

It's separate but likely requires common code changes

rarkins commented 3 years ago

BTW GitHub app tokens can't detect their write permissions so this also needs to somehow be disabled in such cases

pret-a-porter commented 3 years ago

So, I am going to solve this issue in the next way:

  1. Add hasWritePermissions(params: PlatformParams): Promise<boolean>; method to each platform
  2. Invoke hasWritePermissions method inside initPlatform
  3. Enrich PlatformParams interface by dryRun property
  4. When dryRun equals true hasWritePermissions will return true without checking existing token, otherwise will check token and make api call specific for each platform
  5. Somehow handle case with GitHub app
rarkins commented 3 years ago

I'm not sure it makes sense at a platform level because write permissions are generally going to be per-repo. I think better to do the logic in initRepo() instead, if dryRun===false

pret-a-porter commented 3 years ago

Yeah, make sense, but the issue, that initPlatform step throw an error even in dryMode when we don't provide token. Should I move this validation logic from initPlatform to initRepo?

rarkins commented 3 years ago

I don't think that's a good idea because it could throw 100s of errors instead of one? Better that dryRun is passed to initPlatform perhaps?

rarkins commented 3 years ago

I just found that GitHub's GraphQL requires authentication to run at all, so "no token if in dry run mode" seems not possible for GitHub. I have marked the previous conversation as Hidden-Resolved.