Skip to content
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

Get entire repo comments on app install #66

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

aleku399
Copy link
Collaborator

@aleku399 aleku399 commented Nov 2, 2018

No description provided.

@aleku399 aleku399 requested a review from epicallan November 2, 2018 10:55
Copy link
Contributor

@epicallan epicallan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this approach of looking at last x number commits is not optimal can you see if you can get root directories in a repo by using content api without passing in a file path.

@epicallan
Copy link
Contributor

PR title is not informative. PR title should be short description of issue you are working on and maybe issue number

@epicallan
Copy link
Contributor

Remember when we have root directories of a repo, we can recursively get files in the entire project

@epicallan epicallan changed the title waiting for reviews #54 Get entire repo comments Nov 12, 2018
src/lib/github/utils.ts Outdated Show resolved Hide resolved
return Promise.all(modifiedFiles);
}

export async function getfileNames(octokit, owner, repo, sha) {
Copy link
Contributor

@epicallan epicallan Nov 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing types

src/lib/github/utils.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,15 @@
/**
* Gets files that have changed in the last commits
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe comment should change. I think it should be something like Get all repo files.

Copy link
Contributor

@epicallan epicallan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach looks good, could you test this out on a repo and get back to me on how it works out .

@epicallan epicallan changed the title Get entire repo comments Get entire repo comments on app install Dec 4, 2018
@aleku399
Copy link
Collaborator Author

aleku399 commented Dec 6, 2018

this method octokit.gitdata.getTree({owner, repo, sha, recursive: 5}); we are only able to recursively get the fileName and fileContent but if we wanted to use our app to be able to push there are some necessary adjustments needed like getting author and htmlUrl

Copy link
Contributor

@epicallan epicallan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a comment please address and ping me after.

import { getBasicRepoProps, getfileNames } from "./utils";

export default async function getFiles(context: Context): Promise<ModifiedFile[]> {
const { owner, repo } = getBasicRepoProps (context);
const sha = context.payload.head_commit.id;
const octokit = context.github;
const treeObject = await octokit.gitdata.getTree({owner, repo, sha, recursive: 5});
return treeObject.tree.map( async (obj) => {
const arrFile: File[] = treeObject.tree.map( async (obj) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arrFile type is wrong. It returns an array of Promises, so its type is rightly

const arrFile: Array<Promise<File[]>> 

To make it File[] you will have to use Promise.all and await on the returned values of treeObject.tree.map function

@epicallan
Copy link
Contributor

epicallan commented Jan 2, 2019

@aleku399 I tried running the app in this branch and discovered a number of issues to work on;

1 - we should listen to installation_repositories event instead of installation https://developer.github.com/webhooks/#events

2 - The data returned from the installation repository events is of a different type from the one we have worked with before. see https://developer.github.com/v3/activity/events/types/#installationrepositoriesevent. This means you may need to write a new function for getting repository properties something like getInstallationProps and drop usage of getBasicProps

3 - Since the installation doesn't have a commit sha you will need to get the latest commit for a repository by querying all commits and picking out the latest for use with the git tree data API.

4 - I think you should eventually break up the function in getFileOnInstall module into smaller functions. For instance, we can have a separate function for getting the latest commit sha and tree data

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants