feat: create a fork with Octokit given a repository name and its corresponding owner #20
Conversation
Codecov Report
@@ Coverage Diff @@
## master #20 +/- ##
==========================================
+ Coverage 37.28% 43.57% +6.28%
==========================================
Files 3 6 +3
Lines 59 179 +120
Branches 0 4 +4
==========================================
+ Hits 22 78 +56
- Misses 37 100 +63
- Partials 0 1 +1
Continue to review full report at Codecov.
|
TomKristie
changed the title
|
|
||
| import {Logger, Octokit} from '../types'; | ||
|
|
||
| interface ForkData { |
There was a problem hiding this comment.
We could probably generalize this to be Repository with owner and repo fields. A general repository type could be used for upstream and origin elsewhere.
There was a problem hiding this comment.
This specific interface is for returning 2 values from the fork function. There's not another instance in the code where just the owner and repo are passed/returned. If I move this to the general src/types then it will not be used by anything.
There was a problem hiding this comment.
My thought is that maybe GitHubContext and/or GitHubContextParams could specify upstream: Repository rather than upstreamOwner/upstreamRepo
There was a problem hiding this comment.
This is a non-blocker on this PR, but is something to consider later.
src/github-handler/fork-handler.ts
Outdated
|
|
||
| /** | ||
| * Fork the GitHub owner's repository | ||
| * If fork already exists no new fork is created and no error occurs |
There was a problem hiding this comment.
If no error is returned, what is returned?
There was a problem hiding this comment.
I have a @returns statement on line 29. I'll also add a commit to update this comment.
There was a problem hiding this comment.
I meant in the case of the fork already existing. Does the GitHub API createFork return the same data if the fork already exists?
There was a problem hiding this comment.
It returns a superset of the original data. I believe it also adds a parent and source object.
There was a problem hiding this comment.
Can you add to this note that it will return the existing fork?
There was a problem hiding this comment.
Just as an FYI what I originally said isn't true. It returns the exact same data each time (except for the updated_at field). It's just the GitHub docs don't include the parent/source object.
| data: responseData, | ||
| }; | ||
| // setup | ||
| const stub = sinon |
There was a problem hiding this comment.
Are we using nock purely to disable net connect? I wonder if we could potentially get away with just using sinon for the time being.
There was a problem hiding this comment.
I asked for that precisely for that reason. Do you have another suggestion?
There was a problem hiding this comment.
Yes. Nock is solely used as a guarantee that we never connect to the internet. Sinon stubs definitely could do the trick, but isn't fool-proof.
@chingor13 thoughts?
chingor13
added
the
automerge
TomKristie
added
the
type: process
Given an owner and a repository, correctly invoke the create fork Octokit function.
The PR implements the basic functionality of a fork source code with basic mock test.
Towards #19