feat: commit and push changes onto a SHA to a target remote #30

TomKristie · 2020-07-13T23:25:33Z

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:

current reference HEAD
an owner
a repository
a branch name
changes
commit message

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

codecov · 2020-07-13T23:26:34Z

Codecov Report

Merging #30 into master will increase coverage by 54.17%.
The diff coverage is 97.34%.

@@             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

Impacted Files	Coverage Δ
src/github-handler/index.ts	`0.00% <0.00%> (ø)`
src/logger/index.ts	`0.00% <0.00%> (ø)`
src/github-handler/fork-handler.ts	`98.07% <98.07%> (ø)`
src/github-handler/branch-handler.ts	`98.91% <98.91%> (ø)`
src/github-handler/commit-and-push-handler.ts	`98.98% <98.98%> (ø)`
src/github-handler/pr-handler.ts	`100.00% <100.00%> (ø)`
src/types/index.ts	`100.00% <100.00%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f268414...6649350. Read the comment docs.

src/types/index.ts

chingor13 · 2020-07-13T23:36:23Z

src/github-handler/commit-and-push-handler.ts

+function genTreeObj(changes: Changes): GitCreateTreeParamsTree[] {
+  const tree: GitCreateTreeParamsTree[] = [];
+  for (const path in changes) {
+    if (!path) continue;


When would path be unset?

It should never be. This is more of a TypeScript stylistic thing I've seen. I will remove it!

src/github-handler/commit-and-push-handler.ts

chingor13 · 2020-07-13T23:39:14Z

src/github-handler/commit-and-push-handler.ts

+  const tree: GitCreateTreeParamsTree[] = [];
+  for (const path in changes) {
+    if (!path) continue;
+    if (changes[path].content) {


Is "" string truthy? If I want an empty string (a.k.a. an empty file), will this instead delete the file?

Yes it would. I'll fix that to allow empty files.

src/github-handler/commit-and-push-handler.ts

chingor13 · 2020-07-13T23:47:41Z

src/types/index.ts

+}
+
+interface FileData {
+  mode: FileMode;


Do we expect all change sources to include the file mode or should it be optional with a default value?

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.

chingor13 · 2020-07-13T23:49:02Z

src/types/index.ts

+  path?: string;
+  mode?: FileMode;
+  type?: 'blob' | 'tree' | 'commit';


Looks like these are always present?

The ? matches the Octokit type definition exactly right now, but I'll change it to support our needs more.

chingor13 · 2020-07-13T23:52:20Z

In general, we still want to camel case abbreviations (e.g. headSha, somethingHead)

test/commit-and-push.ts

src/github-handler/commit-and-push-handler.ts

chingor13 · 2020-07-14T20:49:38Z

src/github-handler/commit-and-push-handler.ts

+ * @returns {Promise<string>} the GitHub tree SHA
+ */
+async function createTree(
+  logger: Logger,


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?

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

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

chingor13

Code generally LGTM - just a few changes on the structure of the tests

chingor13 · 2020-07-14T22:58:15Z

test/commit-and-push.ts

+  beforeEach(() => {
+    stubGetCommit.restore();
+    stubCreateTree.restore();
+    stubCreateCommit.restore();
+    stubUpdateRef.restore();
+  });


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.

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()

chingor13 · 2020-07-15T15:49:04Z

test/commit-and-push.ts

+  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[] = [


Unless these are used across multiple tests within the describe block, but it into the test that uses this test data.

chingor13 · 2020-07-15T15:54:09Z

test/commit-and-push.ts

+    {
+      path: 'baz.exe',
+      mode: '100755',
+      type: 'blob',
+      sha: null,
+    },
+    {
+      path: 'empty.txt',
+      mode: '100644',
+      type: 'blob',
+      content: '',
+    },


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 TreeObjects.

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.

src/github-handler/commit-and-push-handler.ts

feat: commit and push changes onto a SHA to a target remote

f98ce4a

TomKristie requested a review from a team as a code owner July 13, 2020 23:25

TomKristie requested a review from chingor13 July 13, 2020 23:25

chingor13 requested changes Jul 13, 2020

View reviewed changes

TomKristie added 3 commits July 14, 2020 11:36

fix: bug with empty files + style change

027729b

fix: lint

1b714c9

fix: camel casing

9398de4

TomKristie added the type: process A process-related concern. May include testing, release, or the like. label Jul 14, 2020

TomKristie added this to the Open-PR-From-Fork milestone Jul 14, 2020

chingor13 reviewed Jul 14, 2020

View reviewed changes

TomKristie added 3 commits July 14, 2020 18:04

fix: cascading test errors

3a61aa3

fix: easy stub restoration

fe07b9d

fix: lint

215abe0

chingor13 requested changes Jul 15, 2020

View reviewed changes

TomKristie and others added 5 commits July 15, 2020 14:29

fix: test re-org

9286886

fix: remove nested describe blocks

12a84aa

edit: add sandbox

0b054e6

Merge branch 'master' into basic-commit-and-push

50d7296

fix: edited test files because of octokit breaking type change

0c9a624

chingor13 approved these changes Jul 17, 2020

View reviewed changes

chingor13 added the automerge Merge the pull request once unit tests and other checks pass. label Jul 17, 2020

Merge branch 'master' into basic-commit-and-push

6649350

gcf-merge-on-green bot merged commit 8bf1782 into master Jul 17, 2020

gcf-merge-on-green bot deleted the basic-commit-and-push branch July 17, 2020 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: commit and push changes onto a SHA to a target remote #30

feat: commit and push changes onto a SHA to a target remote #30

TomKristie commented Jul 13, 2020

codecov bot commented Jul 13, 2020 •

edited

chingor13 Jul 13, 2020

TomKristie Jul 14, 2020

chingor13 Jul 13, 2020

TomKristie Jul 14, 2020

chingor13 Jul 13, 2020

TomKristie Jul 14, 2020

chingor13 Jul 13, 2020

TomKristie Jul 14, 2020

chingor13 commented Jul 13, 2020

chingor13 Jul 14, 2020

TomKristie Jul 14, 2020

chingor13 Jul 14, 2020

chingor13 left a comment

chingor13 Jul 14, 2020

TomKristie Jul 15, 2020

chingor13 Jul 15, 2020

chingor13 Jul 15, 2020

feat: commit and push changes onto a SHA to a target remote #30

feat: commit and push changes onto a SHA to a target remote #30

Conversation

TomKristie commented Jul 13, 2020

codecov bot commented Jul 13, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chingor13 commented Jul 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chingor13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 13, 2020 •

edited