Skip to content

Github enterprise support #158

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

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

Github enterprise support #158

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
1 change: 1 addition & 0 deletions docs/reference/configuration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ spr uses the following Git configuration values:
| `branchPrefix` | `--branch-prefix` | String used to prefix autogenerated names of pull request branches | | `spr/GITHUB_USERNAME/` |
| `requireApproval` | | If true, `spr land` will refuse to land a pull request that is not accepted | false |
| `requireTestPlan` | | If true, `spr diff` will refuse to process a commit without a test plan | true |
| `githubApiDomain` | | Domain where the Github API can be found. Override for Github Enterprise support. | true | api.github.com


- The config keys are all in the `spr` section; for example, `spr.githubAuthToken`.
Expand Down
22 changes: 21 additions & 1 deletion spr/src/commands/init.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,27 @@ pub async fn init() -> Result<()> {
return Err(Error::new("Cannot continue without an access token."));
}

let github_api_domain = dialoguer::Input::<String>::new()
.with_prompt("Github API domain (override for Github Enterprise)")
.with_initial_text(
config
.get_string("spr.githubApiDomain")
.ok()
.unwrap_or_else(|| "api.github.com".to_string()),
Copy link
Contributor

@jwatzman jwatzman Nov 1, 2023

Choose a reason for hiding this comment

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

It seems we're special-casing api.github.com consistently, almost using it as a sentinel value, as opposed to actually using the value itself. (i.e., every place we read spr.githubApiDomain we check for api.github.com and then do something different if that is the value, as opposed to using that value itself.)

So instead, can we make this an Option<string>, with None meaning "the default non-enterprise", and Some<x> meaning "use x as the enterprise base URL" -- since we are special-casing api.github.com 100% of the time anyway? That seems more consistent with what is actually going on here.

)
.interact_text()?;

config.set_str("spr.githubApiDomain", &github_api_domain)?;

let api_base_url;
if github_api_domain == "api.github.com" {
api_base_url = "https://api.github.com/v3/".into()
Copy link
Contributor

@jtietema jtietema Oct 26, 2023

Choose a reason for hiding this comment

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

This should be without the /v3/ to make it work for me.

Copy link
Contributor

@jwatzman jwatzman Nov 1, 2023

Choose a reason for hiding this comment

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

@andrewhamon can you check if this is correct? I haven't tested this PR personally yet, but it looks like this is the non-enterprise case, so we obviously need to not break that too :)

} else {
api_base_url = format!("https://{github_api_domain}/api/v3/");
};

let octocrab = octocrab::OctocrabBuilder::new()
.base_url(api_base_url)?
.personal_token(pat.clone())
.build()?;
let github_user = octocrab.current().user().await?;
Expand Down Expand Up @@ -121,7 +141,7 @@ pub async fn init() -> Result<()> {

let url = repo.find_remote(&remote)?.url().map(String::from);
let regex =
lazy_regex::regex!(r#"github\.com[/:]([\w\-\.]+/[\w\-\.]+?)(.git)?$"#);
lazy_regex::regex!(r#"[^/]+[/:]([\w\-\.]+/[\w\-\.]+?)(.git)?$"#);
let github_repo = config
.get_string("spr.githubRepository")
.ok()
Expand Down
2 changes: 1 addition & 1 deletion spr/src/commands/list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ pub async fn list(
};
let request_body = SearchQuery::build_query(variables);
let res = graphql_client
.post("https://api.github.com/graphql")
.post(config.api_base_url() + "graphql")
.json(&request_body)
.send()
.await?;
Expand Down
96 changes: 90 additions & 6 deletions spr/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ pub struct Config {
pub branch_prefix: String,
pub require_approval: bool,
pub require_test_plan: bool,
pub github_api_domain: String,
}

impl Config {
Expand All @@ -29,6 +30,7 @@ impl Config {
branch_prefix: String,
require_approval: bool,
require_test_plan: bool,
github_api_domain: String,
) -> Self {
let master_ref = GitHubBranch::new_from_branch_name(
&master_branch,
Expand All @@ -43,15 +45,25 @@ impl Config {
branch_prefix,
require_approval,
require_test_plan,
github_api_domain,
}
}

pub fn pull_request_url(&self, number: u64) -> String {
format!(
"https://github.com/{owner}/{repo}/pull/{number}",
owner = &self.owner,
repo = &self.repo
)
if self.github_api_domain == "api.github.com" {
format!(
"https://github.com/{owner}/{repo}/pull/{number}",
owner = &self.owner,
repo = &self.repo
)
} else {
format!(
"https://{domain}/{owner}/{repo}/pull/{number}",
domain = &self.github_api_domain,
owner = &self.owner,
repo = &self.repo
)
}
}

pub fn parse_pull_request_field(&self, text: &str) -> Option<u64> {
Expand All @@ -66,7 +78,7 @@ impl Config {
}

let regex = lazy_regex::regex!(
r#"^\s*https?://github.com/([\w\-]+)/([\w\-]+)/pull/(\d+)([/?#].*)?\s*$"#
r#"^\s*https?://[^/]+/([\w\-]+)/([\w\-]+)/pull/(\d+)([/?#].*)?\s*$"#
);
let m = regex.captures(text);
if let Some(caps) = m {
Expand Down Expand Up @@ -140,6 +152,17 @@ impl Config {
self.master_ref.branch_name(),
)
}

pub fn api_base_url(&self) -> String {
if self.github_api_domain == "api.github.com" {
"https://api.github.com/".into()
} else {
format!(
"https://{domain}/api/",
domain = &self.github_api_domain,
)
}
}
}

#[cfg(test)]
Expand All @@ -156,6 +179,7 @@ mod tests {
"spr/foo/".into(),
false,
true,
"api.github.com".into(),
)
}

Expand All @@ -169,6 +193,25 @@ mod tests {
);
}

#[test]
fn test_pull_request_url_github_enterprise() {
let gh = crate::config::Config::new(
"acme".into(),
"codez".into(),
"origin".into(),
"master".into(),
"spr/foo/".into(),
false,
true,
"github.acme.com".into(),
);

assert_eq!(
&gh.pull_request_url(123),
"https://github.acme.com/acme/codez/pull/123"
);
}

#[test]
fn test_parse_pull_request_field_empty() {
let gh = config_factory();
Expand Down Expand Up @@ -229,4 +272,45 @@ mod tests {
Some(123)
);
}
#[test]
fn test_parse_pull_request_field_url_github_enterprise() {
let gh = config_factory();

assert_eq!(
gh.parse_pull_request_field(
"https://github.acme.com/acme/codez/pull/123"
),
Some(123)
);
assert_eq!(
gh.parse_pull_request_field(
" https://github.acme.com/acme/codez/pull/123 "
),
Some(123)
);
assert_eq!(
gh.parse_pull_request_field(
"https://github.acme.com/acme/codez/pull/123/"
),
Some(123)
);
assert_eq!(
gh.parse_pull_request_field(
"https://github.acme.com/acme/codez/pull/123?x=a"
),
Some(123)
);
assert_eq!(
gh.parse_pull_request_field(
"https://github.acme.com/acme/codez/pull/123/foo"
),
Some(123)
);
assert_eq!(
gh.parse_pull_request_field(
"https://github.acme.com/acme/codez/pull/123#abc"
),
Some(123)
);
}
}
4 changes: 2 additions & 2 deletions spr/src/github.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ impl GitHub {
};
let request_body = PullRequestQuery::build_query(variables);
let res = graphql_client
.post("https://api.github.com/graphql")
.post(config.api_base_url() + "graphql")
.json(&request_body)
.send()
.await?;
Expand Down Expand Up @@ -440,7 +440,7 @@ impl GitHub {
let request_body = PullRequestMergeabilityQuery::build_query(variables);
let res = self
.graphql_client
.post("https://api.github.com/graphql")
.post(self.config.api_base_url() + "graphql")
.json(&request_body)
.send()
.await?;
Expand Down
8 changes: 7 additions & 1 deletion spr/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,9 @@ pub async fn spr() -> Result<()> {
.get_bool("spr.requireTestPlan")
.ok()
.unwrap_or(true);
let github_api_domain = git_config
.get_string("spr.githubApiDomain")
.unwrap_or_else(|_| "api.github.com".to_string());

let config = spr::config::Config::new(
github_owner,
Expand All @@ -144,6 +147,7 @@ pub async fn spr() -> Result<()> {
branch_prefix,
require_approval,
require_test_plan,
github_api_domain,
);

let git = spr::git::Git::new(repo);
Expand All @@ -158,7 +162,9 @@ pub async fn spr() -> Result<()> {
}?;

octocrab::initialise(
octocrab::Octocrab::builder().personal_token(github_auth_token.clone()),
octocrab::Octocrab::builder()
.base_url(config.api_base_url() + "v3/")?
.personal_token(github_auth_token.clone())
)?;

let mut headers = header::HeaderMap::new();
Expand Down