-
Notifications
You must be signed in to change notification settings - Fork 17
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: commit and push changes onto a SHA to a target remote #30
Conversation
Codecov Report
@@ Coverage Diff @@
## master #30 +/- ##
===========================================
+ Coverage 37.28% 91.46% +54.17%
===========================================
Files 3 9 +6
Lines 59 586 +527
Branches 0 26 +26
===========================================
+ Hits 22 536 +514
- Misses 37 50 +13
Continue to review full report at Codecov.
|
function genTreeObj(changes: Changes): GitCreateTreeParamsTree[] { | ||
const tree: GitCreateTreeParamsTree[] = []; | ||
for (const path in changes) { | ||
if (!path) continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would path be unset?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should never be. This is more of a TypeScript stylistic thing I've seen. I will remove it!
const tree: GitCreateTreeParamsTree[] = []; | ||
for (const path in changes) { | ||
if (!path) continue; | ||
if (changes[path].content) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is ""
string truthy? If I want an empty string (a.k.a. an empty file), will this instead delete the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it would. I'll fix that to allow empty files.
src/types/index.ts
Outdated
} | ||
|
||
interface FileData { | ||
mode: FileMode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expect all change sources to include the file mode or should it be optional with a default value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a great idea. The only downside I can see is that we will diverge from the Octokit/GitHub v3 api design where there is no default value. However users don't need to know that we use Octokit - just our interface, so there shouldn't be a problem.
src/types/index.ts
Outdated
path?: string; | ||
mode?: FileMode; | ||
type?: 'blob' | 'tree' | 'commit'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like these are always present?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ?
matches the Octokit type definition exactly right now, but I'll change it to support our needs more.
In general, we still want to camel case abbreviations (e.g. headSha, somethingHead) |
* @returns {Promise<string>} the GitHub tree SHA | ||
*/ | ||
async function createTree( | ||
logger: Logger, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question for later/refactor: do we expect the logger to be a singleton (which we can import directly from the module) or do we want to pass an instance around everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might depend on whether or not we foresee child logging. It is currently read-only and there currently is no child-logging so it would work. The only downside is that we would write to a global during setup. I'm not sure what best practices are for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is non-blocking on this PR, but something to consider before 1.0.0 as it probably affects the interface of all the methods we are exporting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code generally LGTM - just a few changes on the structure of the tests
test/commit-and-push.ts
Outdated
beforeEach(() => { | ||
stubGetCommit.restore(); | ||
stubCreateTree.restore(); | ||
stubCreateCommit.restore(); | ||
stubUpdateRef.restore(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit of an odd pattern to have a globally set initial stub, then restore in the beforeEach/setup. Normally, you would do stubbing in the before (or in the test) and clean it up in the teardown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an outdated commit and there's no longer a globally set initial stub. All the stubs are local now. The new behaviour is making sure each test has a clean runtime environment by doing sinon.restore()
test/commit-and-push.ts
Outdated
const changes: Changes = new Map(); | ||
changes.set('a/foo.txt', new FileData('Foo content')); | ||
changes.set('b/bar.txt', new FileData(null)); | ||
changes.set('baz.exe', new FileData(null, '100755')); | ||
changes.set('empty.txt', new FileData('')); | ||
|
||
const tree: TreeObject[] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless these are used across multiple tests within the describe
block, but it into the test that uses this test data.
test/commit-and-push.ts
Outdated
{ | ||
path: 'baz.exe', | ||
mode: '100755', | ||
type: 'blob', | ||
sha: null, | ||
}, | ||
{ | ||
path: 'empty.txt', | ||
mode: '100644', | ||
type: 'blob', | ||
content: '', | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're exporting generateTreeObjects
, I might separate each test/edge case for the input file -> TreeObject
. That way it's more explicit that a FileData
with null|empty content should create certain TreeObject
s.
Otherwise, you're only testing then entire createTree
method which could fail for multiple reasons (and makes it harder to debug). But say that both the createTree
test and a test for null content fails, then it's easier to deduce that the null content bug is failing both tests.
This PR implements the basic functionality of creating GitHub trees, a commit for that tree, and updating the latest reference HEAD to point to that tree. It also has basic mock tests.
Given:
On success, create a tree with those changes, a commit pointing to that tree, and update the reference branch HEAD to point to that new commit.
On error re-throw Octokit Error.
Towards #19