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

checkout v2 #70

Merged
merged 1 commit into from
Dec 3, 2019
Merged

Conversation

ericsciple
Copy link
Contributor

No description provided.

import * as io from '@actions/io';
import * as path from 'path';

export interface IGitCommandManager {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a wrapper over git.exe and exposes methods that correlate 1-to-1 with the git commands we need to run.

tryReset(): Promise<boolean>;
}

export async function CreateCommandManager(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need to do some async stuff when initializing the GitCommandManager class, so this function is exposed and the ctor for GitCommandManager is private


let authConfigKey = `http.https://github.com/.extraheader`;

export async function getSource(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here is the source provider. this basically drives the business logic

src/checkout.ts Outdated
import * as path from 'path';

async function run() {
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

add comment here about using error to break out of the method since there are methods below with side effects like error conditions that we expect to happen and stop continuation (and that is fully okay)

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 dont understand what the concern is?

note, i renamed the file to main.ts so it should be more clear this is the outer try/catch

@@ -0,0 +1,58 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

copied from actions/toolkit

@@ -0,0 +1,11 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

copied from actions toolkit

on:
pull_request:
push:
branches:
- master
- 'releases/*'
- users/ericsciple/action
Copy link
Contributor Author

@ericsciple ericsciple Nov 22, 2019

Choose a reason for hiding this comment

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

remove this line after done testing

src/checkout.ts Outdated
path.join(__dirname, 'problem-matcher.json')
)

// Get sources
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add persist-credentials input

@@ -0,0 +1,77 @@
import * as fs from 'fs'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

some useful helpers i think we should add to the toolkit.... separate pr


<!-- start usage -->
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 added a script to regenerate this from the action.yml

the CI verifies the docs are up to date

//
// This script rebuilds the usage section in the README.md to be consistent with the action.yml

function updateUsage(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we may want to move this function into a toolkit module

i added this based on feedback that the README.md should contain the inputs.

I didnt want to have to deal with maintaining both places, so now the CI runs the npm run gendocs and fails if it isnt up to date

}
}

checkMinimum(minimum: GitVersion): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add access modifiers to these methods to make them clear

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 however the prettier config didnt like it.

lfs: boolean,
accessToken: string
): Promise<void> {
core.info(`Syncing repository: ${repositoryOwner}/${repositoryName}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

you're missing a lot of line ending semicolons. not sure if this is intentional or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's the prettier config. personally i think its weird but i just copied the config from the toolkit 🤷‍♂

@TingluoHuang
Copy link
Member

LGTM 👍

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.

3 participants