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

fallback to REST API to download repo #104

Merged
merged 4 commits into from
Dec 12, 2019

Conversation

ericsciple
Copy link
Contributor

@ericsciple ericsciple commented Dec 10, 2019

@ericsciple ericsciple force-pushed the users/ericsciple/m163tarball branch 2 times, most recently from a60eac9 to 1e6a918 Compare December 10, 2019 22:44
@@ -150,22 +155,10 @@ class GitCommandManager {
args.push(arg)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored most of the lines below into a retry helper. I needed to add retry logic to a new place.

@@ -188,22 +181,10 @@ class GitCommandManager {
async lfsFetch(ref: string): Promise<void> {
const args = ['lfs', 'fetch', 'origin', ref]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored most of the lines below into a retry helper. I needed to add retry logic to a new place.

@@ -381,16 +358,6 @@ class GitCommandManager {
core.debug(`Set git useragent to: ${gitHttpUserAgent}`)
this.gitEnv['GIT_HTTP_USER_AGENT'] = gitHttpUserAgent
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these two functions were moved into the retry helper

@@ -338,13 +319,9 @@ class GitCommandManager {
}

// Minimum git version
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 just reaction because i needed to export MinimumGitVersion. The previous comments about minimumGitVersion moved to the new field that is exported (top of this file)

git,
settings.repositoryPath,
repositoryUrl,
settings.clean
))
) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To simplify how this function reads, i moved the few lines below (to delete contents) into the prepareExistingDirectory function

await io.rmRF(path.join(settings.repositoryPath, file))
}
}

Copy link
Contributor Author

@ericsciple ericsciple Dec 10, 2019

Choose a reason for hiding this comment

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

The rest of these red lines were just indented.

The new logic now:

if (!git)
{
  download using REST
}
else
{
  do the existing logic
}

)
}

// Remove possible previous extraheader
await removeGitConfig(git, authConfigKey)
if (!git) {
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 if (!git) { block is new and just calls the new helper to download the files using the REST API

settings.repositoryPath
)
} else {
// Save state for POST action
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 save-state line was further below, i just moved it up a little and add a stateHelper abstraction

@@ -146,79 +149,110 @@ export async function cleanup(repositoryPath: string): Promise<void> {
await removeGitConfig(git, authConfigKey)
}

async function tryPrepareExistingDirectory(
async function getGitCommandManager(
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 moved some existing logic into this function so the caller's function reads easier.

this function just instantiates the git command manager (wrapper over git executable)

settings.lfs
)
} catch (err) {
// Git is required for LFS
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 try catch is new. As long as the user doesnt have lfs: true, then we can fallback to using the REST API instead

git: IGitCommandManager,
repositoryPath: string,
repositoryUrl: string,
clean: boolean
): Promise<boolean> {
): Promise<void> {
Copy link
Contributor Author

@ericsciple ericsciple Dec 10, 2019

Choose a reason for hiding this comment

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

i refactored this function a little bit

it now handles the case where git is null (ie not using git executable, instead using REST API)

it also performs the delete now too, if the repo folder needs to be recreated. before, the caller function performed the delete (so responsibility just moved)

const defaultMaxAttempts = 3
const defaultMinSeconds = 10
const defaultMaxSeconds = 20

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 had two places with retry logic, and i needed to add a third.

so i went ahead and added a helper class

return new Promise(resolve => setTimeout(resolve, seconds * 1000))
}
}

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 the convenience function. its just a tiny wrapper over the class.

the only reason i export the class is for unit testing

@@ -0,0 +1,29 @@
import * as core from '@actions/core'
import * as coreCommand from '@actions/core/lib/command'

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 simple module to make the caller code read more cleanly

this module abstracts the intra-state stuff (pretty simple logic, but reads gross in the other files, so abstracted behind this file instead)

import {ReposGetArchiveLinkParams} from '@octokit/rest'

const IS_WINDOWS = process.platform === 'win32'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pretty simple function:

  • Download the archive
  • Write to disk
  • Extract
  • Move the files to the correct place (the archive contains an extra top level folder that we need to flatten)

@ericsciple ericsciple merged commit a572f64 into master Dec 12, 2019
This was referenced Mar 12, 2021
@Hamdzadprst00
Copy link

@Hamdzadprst00
Copy link

#104 (comment)

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.

Document the minimum dependencies necessary to use this action with custom containers
3 participants