-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Add ability to use private github repos for recipes #3074
Add ability to use private github repos for recipes #3074
Conversation
Interesting feature thanks.
Let me know if you think that would be a good enough alternative.
|
Hi, thanks for looking at it. Do python recipes already support getting the repository this way via 'git clone'? I only see https download support in the PythonRecipe.
We need this to be automated for our build process, not done by hand as a separate process. Regarding the hardcoded environment variable- I noticed the PythonRecipe already has a provision for using properties based on enviornment variables:
In my case, my recipe just gets the environment variable and sets it within the recipe, so it is not hard coded. Overall, if Python recipes support getting the repo via https, then it should also support getting private repos using the same mechanism, by providing the access token. Edit: |
Yes git cloning is supported in
Good catch for environment variable based properties. @property
def url(self):
key = 'URL_' + self.name
return environ.get(key, self._url) I'm wondering if you could leverage that already to build to clone URL dynamically and add the token like we mentioned?
Yes I get that, that's what I meant, it's in the advantage list of your approach.
Yes agree, one thing however is this is tailor made for GitHub only rather than a generic (header based) authentication. |
Thanks for the feedback. Perhaps we make it generic and let people specify the authorization token and not make it github specific. Would that seem better? |
Yes I was thinking maybe generic header from environment variables could be an option too. |
Ok, I've updated it to specify headers generically, and enhanced the documentation as well: e4ade43 I think this is a pretty clean implementation, thanks for the feedback! |
@AndreMiras I also found applied a tiny unrelated fix around a possible missed python 2.7 migration issue: 254440d |
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.
Beautiful thanks 🙏
It's not clear to me how the environment variable would make it possible to deal with the GitHub use case of 2 headers.
[('Authorization', 'token <your personal access token>'), ('Accept', 'application/vnd.github+json')]
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.
Sweet thanks I think we're covered 🙏
Would it be too much to ask to get a test in tests/test_recipe.py
for the environment variable case? 😬
I think something like that should do it:
...
def test_recipe_download_headers(self):
"""Download header can be created on the fly using environment variables."""
recipe = DummyRecipe()
with mock.patch.dict(os.environ, <do-the-magic>):
download_headers = recipe.download_headers
assert download_headers == [<the-headers>]
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.
Thanks for all the follow up 🙏
This looks good to me.
Let's wait for the CI to complete before we can squash & merge
concurrency: | ||
group: build-${{ github.ref }} | ||
cancel-in-progress: true |
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.
Nice addition thanks
@AndreMiras thanks, happy to help! Looks like all checks have passed. |
Add ability to specify a github_access_token so recipes can download from private github repositories.