Skip to content
This repository has been archived by the owner on Oct 12, 2023. It is now read-only.

Added functionality to pass in github auth_token #50

Merged
merged 3 commits into from Aug 16, 2017
Merged

Added functionality to pass in github auth_token #50

merged 3 commits into from Aug 16, 2017

Conversation

dustindall
Copy link
Contributor

Added install_github auth_token as an environment variable. This allows one to install private github repositories on the Azure cluster (I needed this). I would have passed it into the install_github function but there is a bug that does not pass the auth_token to additional remotes which I needed (#87 under remotes).

@msftclas
Copy link

@dustindall,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
Microsoft Pull Request Bot

@msftclas
Copy link

@dustindall, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

Copy link
Contributor

@paselem paselem 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 the contribution. I have a minor comment regarding your changes. Can you please review?

R/cluster.R Outdated
@@ -84,7 +84,8 @@ generateClusterConfig <- function(fileName, ...){
),
rPackages = list(
cran = vector(),
github = vector()
github = vector(),
authToken = NULL
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the authToken only for gitHub? If so, I would rename to githubAuthToken or consider making this a complex object (probably not ideal for backwards compatibility).

@brnleehng - thoughts on this?

Also, we probably should update some docs to help people understand this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the token is only for github. Also, I changed authToken to githubAuthToken and pushed it out.

Copy link
Contributor

@paselem paselem left a comment

Choose a reason for hiding this comment

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

Minor nitpick, and sorry I should have mentioned this before. Can you please use the full name for the property for clarity:
'gitHubAuthenticationToken'

Approved.

@dustindall
Copy link
Contributor Author

Can we call it githubAuthenticationToken? In the config file under rPackages, github is all lower case and it would be a small nuance to have the H capitalized in one but not the other.

@paselem
Copy link
Contributor

paselem commented Aug 15, 2017

Yes, that seems reasonable to me. Thanks for the attention to detail :).

@dustindall
Copy link
Contributor Author

Made the change

@paselem paselem merged commit 6b215a2 into Azure:master Aug 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants