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

convert SSH URL to HTTPS #179

Merged
merged 1 commit into from
Mar 10, 2020
Merged

Conversation

ericsciple
Copy link
Contributor

@ericsciple ericsciple commented Mar 6, 2020

related to ADR #178

@ericsciple
Copy link
Contributor Author

@m-hilgendorf can you try this branch actions/checkout@users/ericsciple/m167instead_impl

I think it should work for you with the change in this PR.

@ericsciple
Copy link
Contributor Author

@m-hilgendorf sorry i just saw your comment on the other issue... Since you are using private submodules, you would need to also specify the token input to provide a PAT

@m-hilgendorf
Copy link

m-hilgendorf commented Mar 6, 2020

Thank you! Sorry for the trouble.

This PR does remove the need for the git config --global hack.

@ericsciple ericsciple force-pushed the users/ericsciple/m167instead_impl branch from 0c0c1a1 to 71151be Compare March 6, 2020 22:32
@@ -35,7 +35,7 @@ jobs:
uses: actions/checkout@v2

# Basic checkout
- name: Basic checkout
- name: Checkout basic
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check status reads better like this - skim easier since lines up with Verify ___ steps

uses: ./
with:
ref: test-data/v2/submodule
ref: test-data/v2/submodule-ssh-url
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 E2E tests the behavior url.https://github.com/.insteadOf git@github.com:

yield this.configureToken(newGitConfigPath, true);
// Configure HTTPS instead of SSH
Copy link
Contributor Author

Choose a reason for hiding this comment

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

during the submodule update

`git config --local "${this.insteadOfKey}" "${this.insteadOfValue}"`,
`git config --local --show-origin --name-only --get-regexp remote.origin.url`
];
const output = yield this.git.submoduleForeach(commands.join(' && '), this.settings.nestedSubmodules);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

when persisting creds for submodules

const output = yield this.git.submoduleForeach(`git config "${this.tokenConfigKey}" "${this.tokenPlaceholderConfigValue}" && git config --local --show-origin --name-only --get-regexp remote.origin.url`, this.settings.nestedSubmodules);
const commands = [
`git config --local "${this.tokenConfigKey}" "${this.tokenPlaceholderConfigValue}"`,
`git config --local "${this.insteadOfKey}" "${this.insteadOfValue}"`,
Copy link
Contributor Author

@ericsciple ericsciple Mar 9, 2020

Choose a reason for hiding this comment

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

the diff is hard to read here, but basically the git config insteadOf is new

Copy link
Contributor

@thboop thboop left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -114,8 +122,13 @@ class GitAuthHelper {
// Configure a placeholder value. This approach avoids the credential being captured
// by process creation audit events, which are commonly logged. For more information,
// refer to https://docs.microsoft.com/en-us/windows-server/identity/ad-ds/manage/component-updates/command-line-process-auditing
const commands = [
`git config --local "${this.tokenConfigKey}" "${this.tokenPlaceholderConfigValue}"`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Very Clean 👍

@ericsciple ericsciple merged commit 80602fa into master Mar 10, 2020
@ericsciple ericsciple deleted the users/ericsciple/m167instead_impl branch March 10, 2020 14:45
@ericsciple
Copy link
Contributor Author

related to #116

This was referenced Mar 13, 2021
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.

4 participants