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

feat(cli): cli interface #53

Merged
merged 7 commits into from
Jul 31, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 1 addition & 10 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -164,22 +164,13 @@ The GitHub commit message. Default value is: `'code suggestions'`.
*boolean* <br>
Whether or not to force push a reference with different commit history before the remote reference HEAD. Default value is: `false`.

#### `--files [<file1>...]`
*string* <br>
**Required.** A list of files

**Note:** Use either `--files` or `--git-dir` exclusively. Using both with terminate with an error.

#### `--git-dir`
*string* <br>
**Required.** The path of a git directory

**Note:** Use either `--files` or `--git-dir` exclusively. Using both with terminate with an error.


### Example
```
code-suggester pr -o 'Foo' -r 'Bar' --git-dir="/my-project"
code-suggester pr -o foo -r bar -d 'description' -t 'title' -m 'message' --git-dir=.
```

## Supported Node.js Versions
Expand Down
8 changes: 7 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
"engines": {
"node": ">=8.10.0"
},
"bin": {
"code-suggest": "./build/src/bin/code-suggester.js"
},
"repository": "googleapis/code-suggester",
"main": "build/src/index.js",
"module": "build/src/index.js",
Expand Down Expand Up @@ -39,7 +42,10 @@
},
"dependencies": {
"@octokit/rest": "^18.0.1",
"pino": "^6.3.2"
"@types/yargs": "^15.0.5",
"glob": "^7.1.6",
"pino": "^6.3.2",
"yargs": "^15.4.1"
},
"devDependencies": {
"@types/chai": "^4.2.11",
Expand Down
106 changes: 106 additions & 0 deletions src/bin/code-suggester.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
#!/usr/bin/env node

// Copyright 2020 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import * as yargs from 'yargs';
import {CREATE_PR_COMMAND, main} from './workflow';

// tslint:disable:no-unused-expression
// yargs actually is a used expression. TS-lint does not detect it.
yargs
.scriptName('code-suggest')
.usage('$0 pr [args]')
.command(CREATE_PR_COMMAND, 'Create a new pull request', {
'upstream-repo': {
alias: 'r',
demandOption: true,
describe: 'Required. The repository to create the fork off of.',
type: 'string',
},
'upstream-owner': {
alias: 'o',
demandOption: true,
describe: 'Required. The owner of the upstream repository.',
type: 'string',
},
description: {
alias: 'd',
demandOption: true,
describe: 'Required. The GitHub Pull Request description',
type: 'string',
},
title: {
alias: 't',
demandOption: true,
describe: 'Required. The title of the Pull Request.',
type: 'string',
},
branch: {
alias: 'b',
describe: 'The name of the working branch to apply changes to.',
default: 'code-suggestion',
type: 'string',
},
message: {
alias: 'm',
demandOption: true,
describe: 'Required. The GitHub commit message.',
type: 'string',
},
primary: {
alias: 'p',
describe:
"The primary upstream branch to open a Pull Request against. Default is 'master'.",
default: 'master',
type: 'string',
},
force: {
alias: 'f',
describe:
'Whether or not to force push the current reference HEAD against the remote reference HEAD. Default is false.',
default: false,
type: 'boolean',
},
'maintainers-can-modify': {
alias: 'modify',
describe:
'Whether or not maintainers can modify the pull request. Default is true.',
default: true,
type: 'boolean',
},
'git-dir': {
describe:
'Required. The location of any un-tracked changes that should be made into a pull request. Files in the .gitignore are ignored.',
type: 'string',
demandOption: true,
},
})
.check(argv => {
for (const key in argv) {
if (typeof argv[key] === 'string' && !argv[key]) {
throw Error(
`String parameters cannot be provided empty values. Parameter ${key} was given an empty value`
);
}
}
return true;
})
.demandCommand(1, 'A minimum of 1 command must be specified')
.help().argv;

/**
* Parse yargs, get change object, invoke framework-core library!
*/
main();
221 changes: 221 additions & 0 deletions src/bin/handle-git-dir-change.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,221 @@
// Copyright 2020 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import {execSync} from 'child_process';
import {Changes, FileMode, FileData} from '../types';
import {logger} from '../logger';
import {readFile} from 'fs';
import * as path from 'path';

// data about a file in a git directory
interface GitFileData {
path: string;
fileData: FileData;
}

// See https://git-scm.com/docs/git-diff#Documentation/git-diff.txt-git-diff-filesltpatterngt82308203
type GitDiffStatus = 'A' | 'D' | 'M' | 'T' | 'U' | 'X' | string;

class InstallationError extends Error {
constructor(message: string) {
super(message);
this.name = 'InstallationError';
}
}

/**
* Get the absolute path of a relative path
* @param {string} dir the wildcard directory containing git change, not necessarily the root git directory
* @returns {string} the absolute path relative to the path that the user executed the bash command in
*/
export function resolvePath(dir: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function doesn't seem to be adding much, any reason we shouldn't just use path.resolve wherever this is being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I separated it out so that I can explicitly test that absolute paths are generated the way I want to.

const absoluteDir = path.resolve(process.cwd(), dir);
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is only used for the CLI then implicitly depending on the cwd may be ok. If this will be included later in the core (which I could potentially see), we instead want the relative root to be explicit (and provided by the caller - e.g. the CLI).

Copy link
Contributor Author

@TomKristie TomKristie Jul 31, 2020

Choose a reason for hiding this comment

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

this is explicitly for the CLI. I've intended the bin directory to be strictly for the CLI. If we want to do work for the framework-core it will not be handled here. It is a small function so if we wanted to have similar behaviour in the framework-core it might make more sense to duplicate the work.

return absoluteDir;
}

/**
* Get the git root directory.
* Errors if the directory provided is not a git directory.
* @param {string} dir an absolute directory
* @returns {string} the absolute path of the git directory root
*/
export function findRepoRoot(dir: string): string {
try {
return execSync('git rev-parse --show-toplevel', {cwd: dir})
.toString()
.trimRight(); // remove the trailing \n
} catch (err) {
logger.error(`The directory provided is not a git directory: ${dir}`);
throw err;
}
}

/**
* Returns the git diff old/new mode, status, and path. Given a git diff.
* Errors if there is a parsing error
* @param {string} gitDiffPattern A single file diff. Renames and copies are broken up into separate diffs. See https://git-scm.com/docs/git-diff#Documentation/git-diff.txt-git-diff-filesltpatterngt82308203 for more details
* @returns indexable git diff fields: old/new mode, status, and path
*/
function parseGitDiff(
gitDiffPattern: string
): {
oldMode: FileMode;
newMode: FileMode;
status: GitDiffStatus;
relativePath: string;
} {
try {
const fields: string[] = gitDiffPattern.split(' ');
const newMode = fields[1] as FileMode;
const oldMode = fields[0].substring(1) as FileMode;
const statusAndPath = fields[4].split('\t');
const status = statusAndPath[0] as GitDiffStatus;
const relativePath = statusAndPath[1];
return {oldMode, newMode, status, relativePath};
} catch (err) {
logger.warning(
`\`git diff --raw\` may have changed formats: \n ${gitDiffPattern}`
);
throw err;
}
}

/**
* Get the GitHub mode, file content, and relative path asynchronously
* Rejects if there is a git diff error, or if the file contents could not be loaded.
* @param {string} gitRootDir the root of the local GitHub repository
* @param {string} gitDiffPattern A single file diff. Renames and copies are broken up into separate diffs. See https://git-scm.com/docs/git-diff#Documentation/git-diff.txt-git-diff-filesltpatterngt82308203 for more details
* @returns {Promise<GitFileData>} the current mode, the relative path of the file in the Git Repository, and the file status.
*/
export function getGitFileData(
gitRootDir: string,
gitDiffPattern: string
): Promise<GitFileData> {
return new Promise((resolve, reject) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would async/await be easier here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I felt it was easier to explicitly be able to get the readfile error for clearer error messaging. Otherwise I'd have to have nested try catch blocks if i wanted more precise error logging.

try {
const {oldMode, newMode, status, relativePath} = parseGitDiff(
gitDiffPattern
);
// if file is deleted, do not attempt to read it
if (status === 'D') {
resolve({path: relativePath, fileData: new FileData(null, oldMode)});
} else {
// else read the file
readFile(
gitRootDir + '/' + relativePath,
{
encoding: 'utf-8',
},
(err, content) => {
if (err) {
logger.error(
`Error loading file ${relativePath} in git directory ${gitRootDir}`
);
reject(err);
}
resolve({
path: relativePath,
fileData: new FileData(content, newMode),
});
}
);
}
} catch (err) {
reject(err);
}
});
}

/**
* Get all the diffs using `git diff` of a git directory.
* Errors if the git directory provided is not a git directory.
* @param {string} gitRootDir a git directory
* @returns {string[]} a list of git diffs
*/
export function getAllDiffs(gitRootDir: string): string[] {
execSync('git add -A', {cwd: gitRootDir});
const diffs: string[] = execSync('git diff --raw --staged --no-renames', {
cwd: gitRootDir,
})
.toString() // strictly return buffer for mocking purposes. sinon ts doesn't infer {encoding: 'utf-8'}
.trimRight() // remove the trailing new line
.split('\n');
return diffs;
}

/**
* Get the git changes of the current project asynchronously.
* Rejects if any of the files fails to load (if not deleted),
* or if there is a git diff parse error
* @param {string[]} diffs the git diff raw output (which only shows relative paths)
* @param {string} gitDir the root of the local GitHub repository
* @returns {Promise<Changes>} the changeset
*/
export async function parseChanges(
diffs: string[],
gitDir: string
): Promise<Changes> {
try {
// get updated file contents
const changes: Changes = new Map();
const changePromises: Array<Promise<GitFileData>> = [];
for (let i = 0; i < diffs.length; i++) {
// TODO - handle memory constraint
changePromises.push(getGitFileData(gitDir, diffs[i]));
}
const gitFileDatas = await Promise.all(changePromises);
for (let i = 0; i < gitFileDatas.length; i++) {
changes.set(gitFileDatas[i].path, gitFileDatas[i].fileData);
}
return changes;
} catch (err) {
logger.error('Error parsing git changes');
throw err;
}
}

/**
* Throws an error if git is not installed
* @returns {void} void if git is installed
*/
function validateGitInstalled(): void {
try {
execSync('git --version');
} catch (err) {
logger.error('git not installed');
throw new InstallationError(
'git command is not recognized. Make sure git is installed.'
);
}
}

/**
* Load the change set asynchronously.
* @param dir the directory containing git changes
* @returns {Promise<Changes>} the change set
*/
export function getChanges(dir: string): Promise<Changes> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export function getChanges(dir: string): Promise<Changes> {
export async function getChanges(dir: string): Promise<Changes> {

Should this be marked async?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not necessarily. It's only required when we have an await statement.

try {
validateGitInstalled();
const absoluteDir = resolvePath(dir);
const gitRootDir = findRepoRoot(absoluteDir);
chingor13 marked this conversation as resolved.
Show resolved Hide resolved
const diffs = getAllDiffs(gitRootDir);
return parseChanges(diffs, gitRootDir);
} catch (err) {
if (!(err instanceof InstallationError)) {
logger.error('Error loadng git changes.');
}
throw err;
}
}
Loading