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

[vcpkg_from_github] Allow targeting Github Enterprise instances #5719

Merged
merged 2 commits into from
Nov 22, 2019

Conversation

donny-dont
Copy link
Contributor

Allow setting of a URL so vcpkg can download repositories hosted on GitHub Enterprise
instances. Additionally provides a way to set the authorization token when downloading
private repositories.

Allow setting of a URL so vcpkg can download repositories hosted on GitHub Enterprise
instances. Additionally provides a way to set the authorization token when downloading
private repositories.
@donny-dont
Copy link
Contributor Author

donny-dont commented Mar 18, 2019

I haven't written the documentation for the new values yet. I wanted to know if there was any preference to the variable names. Currently its GITHUB_URL which matches whats in the GitLab equivalent. Also I was just using TOKEN for the OAuth token. If there are any preferences here for names let me know. I wasn't sure if either of those aren't specific enough.

@donny-dont
Copy link
Contributor Author

@vicroms not sure what the best naming of things are.

My only concern with GITHUB_URL is that it might imply that you need to set that which is not the case. GITHUB_ENTERPRISE_URL might be better?

With TOKEN I'm just not sure if that's clear enough. Maybe ACCESS_TOKEN? Whatever the consensus is it should probably be added to the gitlab command. I don't use GitLab so I wouldn't know how to check that though.

@donny-dont
Copy link
Contributor Author

@Rastaban @ras0219-msft any feedback on this?

@donny-dont
Copy link
Contributor Author

@vicroms @Rastaban @ras0219-msft is this something that you all don't want to support? The PR has been open for almost 2 months now and I'm not sure what is going on.

@donny-dont
Copy link
Contributor Author

@vicroms @Rastaban @ras0219 any comments?

@donny-dont
Copy link
Contributor Author

Hey @vicroms @Rastaban @ras0219-msft this PR has been open for over 5 months any feedback?

Rename TOKEN -> AUTHORIZATION_TOKEN
@ras0219-msft
Copy link
Contributor

Thank you for your patience and thank you for the PR!

The design LGTM; I renamed the two parameters to GITHUB_HOST and AUTHORIZATION_TOKEN to be slightly more descriptive. I've also added basic documentation.

Assuming no CI issues, this is ready to merge.

@ras0219-msft ras0219-msft changed the title Add URL for targeting Github Enterprise instances [vcpkg_from_Github] Allow targeting Github Enterprise instances Nov 22, 2019
@ras0219-msft ras0219-msft changed the title [vcpkg_from_Github] Allow targeting Github Enterprise instances [vcpkg_from_github] Allow targeting Github Enterprise instances Nov 22, 2019
@ras0219-msft ras0219-msft merged commit ae74e0b into microsoft:master Nov 22, 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.

3 participants