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 hostname support for Enterprise installations of GitHub #118

Merged
merged 5 commits into from
Jul 16, 2019

Conversation

v2kiran
Copy link
Contributor

@v2kiran v2kiran commented Jul 2, 2019

Per the documentation, enterprise installations support a base URL of http(s)://[hostname]/api/v3 in addition to the existing support of http(s)://api.[hostname]/. This PR updates the module to always use the http(s)://[hostname]/api/v3 syntax when [hostname] is not github.com.

Fixes #117

Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

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

Please re-submit without all the miscellaneous formatting changes.

@Cellivar - this will be a breaking change for you (you'll have to update your instances to specify api. with your hostname.

@GitTill
Copy link

GitTill commented Jul 9, 2019

remarks regarding this code change:

GitLab API Version (and url) is updated to V4 -> https://docs.gitlab.com/ee/api/v3_to_v4.html. This change is still using V3 (not supported in GitLab 11.0 and higher). Just changing the URL to v4 results in authentication errors in my environment (tested with valid token):

404 Not Found
This typically happens when the current user isn't properly authenticated. You may need an Access Token with additional scopes checked.

This change is only supporting https - but not http. Support for http would be great.

@HowardWolosky
Copy link
Member

HowardWolosky commented Jul 9, 2019

remarks regarding this code change:

GitLab API Version (and url) is updated to V4
...

@GitTill - The GitLab and GitHub API's are quite different. This module is not designed to work with GitLab...only GitHub.

I took a look at their website and found a reference to PSGitlab as an unofficial PowerShell client for their API. You may want to look at that module.

Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the formatting issues.
Minor issues remain, and then we'll be good to go.
Thanks!

@@ -0,0 +1,4 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

At present time, I don't want to check-in editor-specific files into the project. I realize that you added this as a result of needing to undo your other formatting changes , and I'm open to being convinced otherwise, but at the moment I'm not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah ...how do i remove the file...just add it to gitignore?

Copy link
Member

Choose a reason for hiding this comment

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

  1. delete the file
  2. git add .vscode/settings.json
  3. git commit
  4. git push

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks...this is done

GitHubCore.ps1 Outdated
@@ -153,7 +153,14 @@ function Invoke-GHRestMethod

$hostName = $(Get-GitHubConfiguration -Name "ApiHostName")

$url = "https://api.$hostName/$UriFragment"
if ($hostName -eq "github.com")
Copy link
Member

Choose a reason for hiding this comment

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

I have generally been avoiding double-quoted strings unless I am expecting for them to be interpreted due to embedded expressions/variables/etc...

Suggested change
if ($hostName -eq "github.com")
if ($hostName -eq 'github.com')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:) sure...i will change to single

Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

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

This looks good to go. Thanks for the fix!

@HowardWolosky HowardWolosky changed the title fix github ent. api url Fix hostname support for Enterprise installations of GitHub Jul 16, 2019
@HowardWolosky HowardWolosky merged commit f7b956d into microsoft:master Jul 16, 2019
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.

Fix api url for Github Enterprise
3 participants