-
-
Notifications
You must be signed in to change notification settings - Fork 240
fix(preprod): Understand ghe.com as github_enterprise #3127
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
Conversation
To determine vcs_provider we use part of the origin of the URL: https://github.com/foo/bar -> github https://gitlab.com/abcdef -> gitlab This adds special handling for github enterprize remotes. Now instead of: https://foo.ghe.com/foo/bar -> ghe we end up with: https://foo.ghe.com/foo/bar -> github_enterprise as the backend expects.
0d1aa26 to
8ccb16a
Compare
szokeasaurusrex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
|
|
||
| // GitHub Enterprise Server uses *.ghe.com domains | ||
| if trimmed.ends_with(".ghe.com") { | ||
| return "github_enterprise"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some optional improvements we could do here (both can wait for a separate PR):
-
We could extract
"github_enterprise"to a constant. -
We could define an enum for the supported providers, and include a fallback for unknown providers, like so:
enum VcsProvider { Github, GithubEnterprise Gitlab, Other(String), // or &str }
Then, we would return the enum from this method. We would probably need to add some serialization logic on the enum, as well as a method to convert an arbitrary string to the enum (probably a
From<&str>impl works), so I guess there's some added complexity, but it would be clear what providers we support natively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good! will try to send a follow up shortly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! And as I said, I think these are just optional improvements, so no rush. It is also fine if we don't do it.
To determine vcs_provider we use part of the origin of the URL:
https://github.com/foo/bar -> github
https://gitlab.com/abcdef -> gitlab
This adds special handling for github enterprize remotes. Now instead
of:
https://foo.ghe.com/foo/bar -> ghe
we end up with:
https://foo.ghe.com/foo/bar -> github_enterprise
as the backend expects.
Resolves EME-821