Mixed improvements on server and client #27

mcornella · 2016-10-09T17:34:08Z

This PR contains the following improvements:

Add cross-env dependency so that npm start works.
Mark PullRequestEvents as merged when appropriate: as is now, closed and merged PRs have both an action = closed. See the description of the action field in the PullRequestEvent documentation.
Include URL of the specific event when sending it to the client, and display that in the client. This includes all events except PushEvents which don't include an appropriate URL in the API response.

Tested locally.

debugger22 · 2016-10-21T07:21:30Z

app/public/js/main.js

@@ -32,7 +32,7 @@ var svg_background_color_online = '#0288D1',



-var socket = io(document.location.hostname);
+var socket = io(document.location.host);


Thanks for the PR!

Could you please fix the merge conflict here.

The GitHub API doesn't actually return `action = merged' when a PR is merged; it just sets `pull_request.merged = true'. From the Description field for the Action key: https://developer.github.com/v3/activity/events/types/#pullrequestevent

With this commit each event has a proper link directing to the actual event, except PushEvents which direct to the repo since there isn't any available link to the commits.

debugger22 · 2016-10-21T07:30:49Z

That was quick! 😄

mcornella · 2016-10-21T07:33:00Z

I was waiting for it 😊

On another topic, these binaries are huge, it'd be better putting them in another place: maybe using github LFS or just putting them as a release which doesn't involve any additional installation.

In any case that would involve rewriting history to delete them out of the repository, because it has become super-slow to fetch and push due to the extra 80MB. It would probably involve telling people that you rebased master, but since it's 8 days ago it shouldn't have much impact and the benefit is clear.

debugger22 · 2016-10-21T07:34:48Z

Just one catch here. Now push events are redirecting to https://api.github.com URL.

debugger22 · 2016-10-21T07:35:30Z

Maybe that's fine, I'm not sure what users prefer.

mcornella · 2016-10-21T07:43:33Z

Just one catch here. Now push events are redirecting to https://api.github.com URL.

Oh sorry I thought I fixed it. It seems I didn't double check it and the API docs are wrong, there doesn't seem to be any html_url field:

{ id: '4745514621',
  type: 'PushEvent',
  actor: 
   { id: 7099784,
     login: 'docbacardi',
     display_login: 'docbacardi',
     gravatar_id: '',
     url: 'https://api.github.com/users/docbacardi',
     avatar_url: 'https://avatars.githubusercontent.com/u/7099784?' },
  repo: 
   { id: 70711456,
     name: 'muhkuh-sys/org.muhkuh.lua-jonchki',
     url: 'https://api.github.com/repos/muhkuh-sys/org.muhkuh.lua-jonchki' },
  payload: 
   { push_id: 1360737728,
     size: 5,
     distinct_size: 5,
     ref: 'refs/heads/master',
     head: '13ab152ce613758121339bd3a5b0ff1bea5d9790',
     before: '35d914f64b3d61b65d66859a3e4687b339bd4428',
     commits: [ [Object], [Object], [Object], [Object], [Object] ] },
  public: true,
  created_at: '2016-10-21T07:41:23Z',
  org: 
   { id: 7113795,
     login: 'muhkuh-sys',
     gravatar_id: '',
     url: 'https://api.github.com/orgs/muhkuh-sys',
     avatar_url: 'https://avatars.githubusercontent.com/u/7113795?' } }

It should be easy to change just manually concatenating the repo_name, as it was before in the client side:

diff --git a/server/index.js b/server/index.js
index caa879e..36b0774 100755
--- a/server/index.js
+++ b/server/index.js
@@ -117,7 +117,7 @@ function stripData(data){
           'payload_size': data.payload.size,
           'message': data.payload.commits[0].message,
           'created': data.created_at,
-          'url': data.repo.url
+          'url': 'https://github.com/' + data.repo.name
         });
         pushEventCounter++;
       }

debugger22 · 2016-10-21T07:47:34Z

Thanks! I'll do it.

mcornella · 2016-10-21T07:55:58Z

Make sure to remove the binaries, it will be a huge source of pain in the future, that happened to me as well. git rebase and delete the commits, don't make a git rm commit because the size will still be there in the earlier commits. Right now not many forks have these commits so it'll be easy making a push force. Use github releases to store the binaries instead.

debugger22 · 2016-10-21T07:58:06Z

Did you mean to remove the binary files and rebase to the commit prior to the commit having binary files?

mcornella · 2016-10-21T08:05:33Z

No, that won't work. You have to make an interactive rebase to the commit prior to the one having binary files. git rebase -i 864d3ea74^ and edit the commits where you introduced them.

You can use git log --oneline --name-only to identify which files changed on each commit:

$ git log --oneline --name-only 
212260b Merge pull request #27 from mcornella/mixed-improvements
c5a04f3 Add URL field for each event
app/public/js/main.js
server/index.js
5301419 Set `merged' action if `merged' key is true
server/index.js
9ccb7a3 Add cross-env dependency so that `npm start' works
package.json
bdb0b48 Merge pull request #31 from jennylia/master
5be2b66 added some details to README
.gitignore
README.md
app/public/js/main.js
1a3be3a Update index.html
app/index.html
53961da Add an extra line break for apps text
app/index.html
ecae20e Add app links to website
app/index.html
afaa324 Move bin to static
app/public/bin/GitHub-Audio-v1.0-linux-x64.zip
app/public/bin/GitHub-Audio-v1.0-macOS-x64.zip
864d3ea Add binaries for macOS and Linux
app/index.html
app/public/css/main.css
bin/GitHub-Audio-v1.0-linux-x64.zip
bin/GitHub-Audio-v1.0-macOS-x64.zip
0d9e7ac Fix draw issue on hidden screen
app/public/js/main.js
684dd31 Fix syntax error

It seems you'll only have to edit 864d3ea (Add binaries for macOS and Linux) and afaa324 (Move bin to static). If you're unfamiliar with rebase interactive let me know, but there should be a helpful explanation of how to use it once you run the git rebase -i command.

Let me know if I need to explain further!

Oh, and at some point you'll have to also change the URLs from index.html but that can be done afterwards.

mcornella · 2016-10-21T11:09:25Z

I've found a way to do it programatically better. Make sure you're on the most updated master and you have your zip files backed up somewhere else:

Reconstruct git history ignoring big files (from Purging a file or directory from history):

reconstruct_git_ignoring_file() { git filter-branch -f --prune-empty --index-filter \
"git rm -rf --cached --ignore-unmatch \"$1\"" --tag-name-filter cat -- --all }

reconstruct_git_ignoring_file app/public/bin/GitHub-Audio-v1.0-linux-x64.zip
reconstruct_git_ignoring_file app/public/bin/GitHub-Audio-v1.0-macOS-x64.zip
reconstruct_git_ignoring_file bin/GitHub-Audio-v1.0-linux-x64.zip
reconstruct_git_ignoring_file bin/GitHub-Audio-v1.0-macOS-x64.zip

Delete backup refs, purge and garbage-collect all unreachable commits: those in the master branch, other branches or on tags won't be deleted (from this stackoverflow answer):
```
git for-each-ref --format='delete %(refname)' refs/original | git update-ref --stdin
git reflog expire --expire-unreachable=now --all
git gc --prune=now
```

This reduces significantly the size of the git repository:

$ du -s .git   # before
83M     .git
$ du -s .git   # after
2.5M    .git

Afterwards, you have to make a force push git push -f and that's all. Make sure you merge #26 first or else it will be more difficult solving the conflict.

Mixed improvements on server and client

debugger22 reviewed Oct 21, 2016

View reviewed changes

mcornella added 3 commits October 21, 2016 09:26

Add cross-env dependency so that `npm start' works

9ccb7a3

Set merged' action if merged' key is true

5301419

The GitHub API doesn't actually return `action = merged' when a PR is merged; it just sets `pull_request.merged = true'. From the Description field for the Action key: https://developer.github.com/v3/activity/events/types/#pullrequestevent

Add URL field for each event

c5a04f3

With this commit each event has a proper link directing to the actual event, except PushEvents which direct to the repo since there isn't any available link to the commits.

debugger22 merged commit 212260b into debugger22:master Oct 21, 2016

mcornella deleted the mixed-improvements branch October 21, 2016 07:33

debugger22 added a commit that referenced this pull request Jan 18, 2023

Merge pull request #27 from mcornella/mixed-improvements

fce22e6

Mixed improvements on server and client

debugger22 added a commit that referenced this pull request Jan 18, 2023

Merge pull request #27 from mcornella/mixed-improvements

c9dbaac

Mixed improvements on server and client

debugger22 added a commit that referenced this pull request Jan 18, 2023

Merge pull request #27 from mcornella/mixed-improvements

09c6e86

Mixed improvements on server and client

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Mixed improvements on server and client #27

Mixed improvements on server and client #27

Uh oh!

mcornella commented Oct 9, 2016 •

edited

Loading

Uh oh!

debugger22 Oct 21, 2016

Uh oh!

mcornella Oct 21, 2016

Uh oh!

debugger22 commented Oct 21, 2016

Uh oh!

mcornella commented Oct 21, 2016 •

edited

Loading

Uh oh!

debugger22 commented Oct 21, 2016

Uh oh!

debugger22 commented Oct 21, 2016

Uh oh!

mcornella commented Oct 21, 2016 •

edited

Loading

Uh oh!

debugger22 commented Oct 21, 2016

Uh oh!

mcornella commented Oct 21, 2016

Uh oh!

debugger22 commented Oct 21, 2016

Uh oh!

mcornella commented Oct 21, 2016 •

edited

Loading

Uh oh!

mcornella commented Oct 21, 2016

Uh oh!

Uh oh!

		@@ -32,7 +32,7 @@ var svg_background_color_online = '#0288D1',



		var socket = io(document.location.hostname);
		var socket = io(document.location.host);

Mixed improvements on server and client #27

Mixed improvements on server and client #27

Uh oh!

Conversation

mcornella commented Oct 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

debugger22 Oct 21, 2016

Choose a reason for hiding this comment

Uh oh!

mcornella Oct 21, 2016

Choose a reason for hiding this comment

Uh oh!

debugger22 commented Oct 21, 2016

Uh oh!

mcornella commented Oct 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

debugger22 commented Oct 21, 2016

Uh oh!

debugger22 commented Oct 21, 2016

Uh oh!

mcornella commented Oct 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

debugger22 commented Oct 21, 2016

Uh oh!

mcornella commented Oct 21, 2016

Uh oh!

debugger22 commented Oct 21, 2016

Uh oh!

mcornella commented Oct 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcornella commented Oct 21, 2016

Uh oh!

Uh oh!

mcornella commented Oct 9, 2016 •

edited

Loading

mcornella commented Oct 21, 2016 •

edited

Loading

mcornella commented Oct 21, 2016 •

edited

Loading

mcornella commented Oct 21, 2016 •

edited

Loading