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

set insteadOf url for org-id #621

Merged
merged 2 commits into from Nov 1, 2021
Merged

set insteadOf url for org-id #621

merged 2 commits into from Nov 1, 2021

Conversation

ericsciple
Copy link
Contributor

@ericsciple ericsciple commented Oct 22, 2021

fixes #570

When ssl certificate authentication is enabled, the clone UI suggested in the UI is like org-<ORG_ID>@github.com:<ORG>/<REPO>.git instead of the normal format git@github.com:<ORG>/<REPO>.git

We need to register a git config insteadOf url to handle the new format. Today users set the input submodules: true and the input ssh-key is not provided, we register an insteadOf config so SSH URLs beginning with git@github.com: are converted to HTTPS. We need to do the same thing for the new format.

`git config --local '${this.insteadOfKey}' '${this.insteadOfValue}'`,
this.settings.nestedSubmodules
)
for (const insteadOfValue of this.insteadOfValues) {
Copy link
Contributor Author

@ericsciple ericsciple Nov 1, 2021

Choose a reason for hiding this comment

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

this is for submodule config (recursive submodules)

@@ -63,7 +63,12 @@ class GitAuthHelper {

// Instead of SSH URL
this.insteadOfKey = `url.${serverUrl.origin}/.insteadOf` // "origin" is SCHEME://HOSTNAME[:PORT]
this.insteadOfValue = `git@${serverUrl.hostname}:`
this.insteadOfValues.push(`git@${serverUrl.hostname}:`)
if (this.settings.workflowOrganizationId) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

register extra prefix now

@@ -118,7 +123,9 @@ class GitAuthHelper {
// Configure HTTPS instead of SSH
await this.git.tryConfigUnset(this.insteadOfKey, true)
if (!this.settings.sshKey) {
await this.git.config(this.insteadOfKey, this.insteadOfValue, true)
for (const insteadOfValue of this.insteadOfValues) {
await this.git.config(this.insteadOfKey, insteadOfValue, true, true)
Copy link
Contributor Author

@ericsciple ericsciple Nov 1, 2021

Choose a reason for hiding this comment

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

when configuring auth, now we register multiple instead-of values

this translates submodule URLs like:

git@github.com:my-org/my-submodule.git -> https://github.com/my-org/my-submodule.git
org-123456@github.com:my-org/my-submodule.git -> https://github.com/my-org/my-submodule.git

])
const args: string[] = ['config', globalConfig ? '--global' : '--local']
if (add) {
args.push('--add')
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 required in order to add two insteadOf URLs

Copy link

Choose a reason for hiding this comment

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

Try out running checkout on two separate occasions (i.e. the repo is not deleted in between runs).

/**
* Gets the organization ID of the running workflow or undefined if the value cannot be loaded from the GITHUB_EVENT_PATH
*/
export async function getOrganizationId(): Promise<number | undefined> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

best effort, debug log if can't load

Choose a reason for hiding this comment

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

best effort, debug log if can't load

@juliobbv
Copy link

juliobbv commented Nov 1, 2021

Changes LGTM 👍 . The only thing pending to test is that --add scenario and we're good with merging.

@ericsciple ericsciple merged commit ec3a7ce into main Nov 1, 2021
@ericsciple ericsciple deleted the users/ericsciple/21-10-org branch November 1, 2021 16:43
Copy link

@Oriblish Oriblish left a comment

Choose a reason for hiding this comment

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

test/git-auth-helper.test.ts

/**
* Gets the organization ID of the running workflow or undefined if the value cannot be loaded from the GITHUB_EVENT_PATH
*/
export async function getOrganizationId(): Promise<number | undefined> {

Choose a reason for hiding this comment

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

best effort, debug log if can't load

Copy link

@Oriblish Oriblish left a comment

Choose a reason for hiding this comment

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

test/git-auth-helper.test.ts

@Oriblish
Copy link

#621 (review)

@Oriblish
Copy link

🧩

Copy link

@Oriblish Oriblish left a comment

Choose a reason for hiding this comment

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

test/git-auth-helper.test.ts``

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.

Organization ssh url will not support submodule checkout
3 participants