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

FIX: Private enterprise github references #38

Closed

Conversation

johnthedev97
Copy link

I was redirected from here hashicorp/terraform#9644

Added a few unit test cases for github detector
Removed the hardcoded domain ‘github.com’ in detectSSH() and set it from
the source
Modified the initial testing logic for HTTPS in Detect() such that
handles domain names where github is a subdomain

PS: The test cases was added by looking at the code and behavior before
my code changes and not from knowledge of use cases/expected values. My
main objective for the test cases was to ensure I don’t break the
behavior regular github.com URLs

Added a few unit test cases for github detector
Removed the hardcoded domain ‘github.com’ in detectSSH() and set it from
the source
Modified the initial testing logic for HTTPS in Detect() such that
handles domain names where github is a subdomain

PS: The test cases was added by looking at the code and behavior before
my code changes and not from knowledge of use cases/expected values. My
main objective for the test cases was to ensure I don’t break the
behavior regular github.com URLs
@mitchellh
Copy link
Contributor

This looks good, but not being knowledgeable about this is it a fair assumption that if github is in the domain, that we are talking to a private GitHub instance? I suppose thats more likely than not but I wonder if that is too weird...

@johnthedev97
Copy link
Author

I cannot say for sure, but if github is sub domain (github.company.com) I thought it might be fair assumption. It was in my case (I faced issue accessing my company's private github in terraform module source over ssh)

@johnthedev97
Copy link
Author

Let me know if you have any suggestions on ascertaining the URL refers to github.

I think for SSH, as the user explicitly start with 'git@' it is fine. I mean even if it is not github and is some other git provider it would work.

If you want I can remove the changes for HTTP, as that is more subjective

@moofish32
Copy link

moofish32 commented Mar 22, 2017

@mitchellh - I have the same github enterprise configuration at my company and the same feature request. I could agree on only opening up ssh access, based on git@ and not the http(s) interface. Is there any chance of this making it into master or is the lack of a robust detection mechanism a blocker?

@rbhitchcock
Copy link

rbhitchcock commented Apr 27, 2018

I ran into this issue today while trying to import a Terraform module from my company's Github Enterprise install.

module "foo" {
  source = "git::git@github.company.com/org/modules.git//dag?ref=mybranch"
}

Based on the existing behavior, it treats the source as a local file:// URI as opposed to a git URI. Is there anything I can do to help this change be merged? I'm happy to make any modifications to meet your requirements @mitchellh (assuming @johnthedev97 has moved on to other things and no longer has time for this).

My 2 cents would be to back out the changes for HTTP detection. A workaround for this already exists, e.g. source = git::https://github.saobby.my.eu.orgpany.com/org/modules.git. The changes for SSH detection by looking for git@ instead of git@github seem to be the most important.

Please let me know if there's anything I can do to help this along!

@johnthedev97
Copy link
Author

@rbhitchcock if it helps, I went ahead with accessing github via SSH using the format - "git::ssh://git@github.company.com/...." which works without this patch

@rbhitchcock
Copy link

@johnthedev97 Thank you!

@mitchellh
Copy link
Contributor

This should be fixed by #129 very soon.

I didn't do the magic behavior where if "github" and "com" are in the URL I assume GitHub. However, SCP-like URLs like git@anyhost.com:path will now work as you expect! This should make all the GHE access work.

@mitchellh mitchellh closed this Nov 17, 2018
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