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.

Sign up for GitHub

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

Handle URLs from GitHub Enterprise managed orgs #2098

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Handle URLs from GitHub Enterprise managed orgs #2098

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions R/utils-gh.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,19 @@
prot_host <- strsplit(host_url, "://", fixed = TRUE)[[1]]
if (is_github_dot_com(host_url)) {
paste0(prot_host[[1]], "://api.github.com")
} else if(is_github_enterprise(host_url)) {
paste0(prot_host[[1]], "://api.", prot_host[[2]])

Check warning on line 45 in R/utils-gh.R

View check run for this annotation

Codecov / codecov/patch

R/utils-gh.R#L44-L45 

Added lines #L44 - L45 were not covered by tests
} else {
paste0(host_url, "/api/v3")
}
}

is_github_enterprise <- function(url) {
url <- get_baseurl(url)
url <- normalize_host(url)
grepl("ghe.com$", url)

Check warning on line 54 in R/utils-gh.R

View check run for this annotation

Codecov / codecov/patch

R/utils-gh.R#L52-L54 

Added lines #L52 - L54 were not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

Do you have any concrete info on how universally this applies to GHE? Like, is this scheme for the hostname forced on you or is it a default or recommended? I've long wanted to better account for GHE and dragged my feet due to my perception that this could be anything.

Copy link
Member

Choose a reason for hiding this comment

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

For example, in #1897, the host is ghe-gsk-prod.metworx.com. So I am inclined to believe that full GHE support will need to make this configurable.

Copy link
Author

Choose a reason for hiding this comment

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

I didn't stand-up the environment, so I can't be 100% certain, but my understanding is the following.

  1. Those using the Free, Pro and Team versions of GitHub will use "api.github.com" as normal.
  2. Those using GitHub Enterprise Cloud will use "api.SUBDOMAIN.ghe.com".
  3. Those using GitHub Enterprise Server will be self-hosting and so their API URL can be anything (as I assume is happening in the example of github_remote_list() not picking up GHE host #1897 above).

To confirm item (2) above, you can have a look at this page in the GitHub API documentation which says the following in the first section:

If you access GitHub at GHE.com, replace api.github.com with your enterprise's dedicated subdomain at api.SUBDOMAIN.ghe.com.

If that's true, then you are correct in that the changes in this PR don't amount to full GHE support, because they don't account for self-hosted GitHub Enterprise server. And I agree that the only way to properly account for it would be to make it configurable. But it does feel to me like "ghe.com" is standard enough that it would be nice to account for it automatically without users having to set their own environment variable. Allowing a configurable override would of course be a sensible next step.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the extra info! So I gather that you have GitHub Enterprise Cloud? It does seems like accomodating that would be considerably easier than full configurability. I'm about to be travelling, but I'll come back and review this PR.

Copy link
Author

Choose a reason for hiding this comment

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

So I gather that you have GitHub Enterprise Cloud?

That's correct yes.

I'm about to be travelling, but I'll come back and review this PR.

Safe travels! And thanks for the prompt response!

}

is_github_dot_com <- function(url) {
url <- get_baseurl(url)
url <- normalize_host(url)
Expand Down
2 changes: 1 addition & 1 deletion R/utils-github.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ github_remote_list <- function(these = c("origin", "upstream"), x = NULL) {

parsed <- parse_github_remotes(set_names(x$url, x$name))
# TODO: generalize here for GHE hosts that don't include 'github'
is_github <- grepl("github", parsed$host)
is_github <- grepl("github|ghe", parsed$host)
parsed <- parsed[is_github, ]

parsed$remote <- parsed$name
Expand Down
Loading