-
Notifications
You must be signed in to change notification settings - Fork 993
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
Cascade author details to Azure commit #1253
Conversation
@greysteil after trying what you suggested, I'm still having problems. Please could you take a look and see what you think. If you think there's a genuine gap, then I can look at adding some tests. |
Does the author_details structure match what’s expected by the REST api? https://docs.microsoft.com/en-us/rest/api/azure/devops/git/pushes/create?view=azure-devops-rest-5.0#gituserdate Also make sure you check if author_details has a value, as we don’t pass this around ourselves. |
I've cleaned this up and think it should be good to go now. @acraven can you test it? You'll want to set |
@greysteil I added some thoughts for tests in the last push, but looks like you've covered @chris5287 's concerns. I hadn't realised you were jumping in; it's fine, I can just remove the empty tests unless you think there's value in implementing them. |
@greysteil That's a nice workflow for integration testing. I haven't tested with I had to create another repo that hadn't previously been infected with an empty author.email. Even having deleted the remote and local branches of the rogue PR (without author.email) dependabot failed. I'm happy if you want to remove the empty tests and accept the PR, but please squash merge if you can as my commits look untidy. |
Awesome - I'll take this from here. Can I ask you to sign a doc (attached) that transfers the IP from this contribution to Dependabot? Sorry it's a pain - we need it because we're planning to change the license on this repo (to one with the same intention that's a bit tighter). Dependabot IP Letter - Andrew Craven.pdf If you can email the signed form to support@dependabot.com that would be awesome. Electronic signature is fine. |
790c2ca
to
ed18bd7
Compare
@greysteil Will sort out the doc. In the meantime, please hold off merging, which I guess you will without the signed doc. I'm now running my script in Azure using this PR's Gems but it's not creating the PRs. I'm seeing new branches with commits and these are populated with author name and email as per the update.rb script. It created the PRs when I ran locally. I wonder if the committer email being blank is now a problem. Investigating... |
Right, this doesn't seem solvable by you or me, the PR API is returning 403 I guess this means the PR is okay, but I need to parse the response better (currently just pulling out the PR number) |
@greysteil you should have IP transfer in your inbox. I created by own Azure DevOps org to test this - I found I had to add not only |
I'm not expecting this to be merged, it's just an opener before I spent time learning how to write Ruby/rspec tests.
Having tried passing to author_details to PullRequestCreator I'm still getting a null author email in the Azure DevOps repo, which causes an exception elsewhere in dependabot. The changes are where I think the gaps are.
I can confirm that if I call the Azure API directly to create a commit, the commit author defaults to those relating to the access token. The author fields can be completely or partially supplied and the API fills in as best it can.
It doesn't seem that there is an email associated with the System.AccessToken pipeline variable. Ultimately this appears to be causing my problem.