Skip to content

checkout submodules#173

Merged
ericsciple merged 1 commit into
masterfrom
users/ericsciple/m166submodule_impl
Mar 5, 2020
Merged

checkout submodules#173
ericsciple merged 1 commit into
masterfrom
users/ericsciple/m166submodule_impl

Conversation

@ericsciple

@ericsciple ericsciple commented Mar 2, 2020

Copy link
Copy Markdown
Contributor

Add input to control whether to checkout submodules.

Related to ADR #157

Comment thread README.md Outdated
Comment thread src/git-directory-helper.ts
@ericsciple ericsciple changed the title checkout submodules [draft] checkout submodules Mar 2, 2020
Comment thread src/state-helper.ts Outdated
@ericsciple ericsciple force-pushed the users/ericsciple/m166submodule_impl branch from 744fdac to dbba4e3 Compare March 2, 2020 16:48
await git.remoteAdd('origin', repositoryUrl)
}
return
}

@ericsciple ericsciple Mar 2, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The diff looks pretty bad right here, even though mostly indention just changed.

I made the above if-block return to short circuit.

Since this function is basically a long sequence of git commands, i wanted to de-indent the below code one level so this overall function is a little bit easier to follow.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll leave comments to call out what actually changed

// Checkout
await git.checkout(checkoutInfo.ref, checkoutInfo.startPoint)

// Submodules

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These additional submodule commands are the only thing that changed

shell: bash
run: __test__/verify-lfs.sh

# Submodules false

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i really love being able to e2e validate :)

).toBeGreaterThanOrEqual(0)
})

const configuresAuthHeaderEvenWhenPersistCredentialsFalse =

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this test got renamed to configureAuth_blahblah

}
)

const registersBasicCredentialAsSecret =

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

again, this test got renamed (prefixed)

if (settings.submodules) {
try {
// Temporarily override global config
await authHelper.configureGlobalAuth()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here's the trick for auth:

  • Copy global git config to runner_temp
  • Temporarily override $HOME to point to the temp folder (just during submodule commands)
  • Add auth token to the global git config (under runner temp)

Spent forever trying to figure out how to do this securely. Here's the problems involved:

  • Avoid passing creds on the command line due to customers commonly logging process audit events (dont want creds written to security event log).
  • Avoid cred helper due to issue w/ apple git caching and not calling cred helper
  • Submodules do not use the main repository's local git config, they have their own
  • A submodule's local git config does not exist until submodule update is run (the fetch). So the trick we use when cloning the main repo isnt possible (git init, git remote add, git config auth, then finally git fetch).

@ericsciple ericsciple changed the title [draft] checkout submodules checkout submodules Mar 2, 2020
await this.execGit(args)
}

async submoduleUpdate(fetchDepth: number, recursive: boolean): Promise<void> {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

add retry for submoduleUpdate

@timsnyder-siv

Copy link
Copy Markdown

Hi. Is this PR targeting only the usecase from #166 or is it trying to bring more general submodule support back into checkout@v2?

@ericsciple

Copy link
Copy Markdown
Contributor Author

@timsnyder-siv the goal is to add general submodule support to checkout@v2


// Persist credentials
if (settings.persistCredentials) {
await authHelper.configureSubmoduleAuth()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do we remove the cred during post-job cleanup?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

git-auth-helper.removeAuth() removes it

@ericsciple ericsciple force-pushed the users/ericsciple/m166submodule_impl branch from 14250c9 to 3577377 Compare March 5, 2020 19:15
@ericsciple ericsciple merged commit 422dc45 into master Mar 5, 2020
@ericsciple ericsciple deleted the users/ericsciple/m166submodule_impl branch March 5, 2020 19:22
valfirst added a commit to vividus-framework/vividus that referenced this pull request May 30, 2020
Checkout action introduced number of improvements simplifying checkout of submodules and fetch of all history for all tags and branches:
- actions/checkout#258
- actions/checkout#173
valfirst added a commit to vividus-framework/vividus that referenced this pull request May 30, 2020
Checkout action introduced number of improvements simplifying checkout of submodules and fetch of all history for all tags and branches:
- actions/checkout#258
- actions/checkout#173
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