Skip to content

Add ability to use private github repos for recipes#3074

Merged
AndreMiras merged 14 commits into
kivy:developfrom
autosportlabs:brent/private_github_repos
Oct 22, 2024
Merged

Add ability to use private github repos for recipes#3074
AndreMiras merged 14 commits into
kivy:developfrom
autosportlabs:brent/private_github_repos

Conversation

@brentpicasso

Copy link
Copy Markdown
Contributor

Add ability to specify a github_access_token so recipes can download from private github repositories.

@AndreMiras

Copy link
Copy Markdown
Member

Interesting feature thanks.
Alternatively you should already be able to clone using the token like so:

git clone https://$TOKEN@github.com/$USERNAME/$REPOSITORY.git

Let me know if you think that would be a good enough alternative.
Your approach has a couple of advantages over this one however:

  • no full clone needed
  • no hardcoded credentials in the recipe (as it can be in the environment variable)

@brentpicasso

brentpicasso commented Oct 18, 2024

Copy link
Copy Markdown
Contributor Author

Interesting feature thanks. Alternatively you should already be able to clone using the token like so:

git clone https://$TOKEN@github.com/$USERNAME/$REPOSITORY.git

Let me know if you think that would be a good enough alternative. Your approach has a couple of advantages over this one however:

* no full clone needed

* no hardcoded credentials in the recipe (as it can be in the environment variable)

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.

git clone https://$TOKEN@github.com/$USERNAME/$REPOSITORY.git

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:

 @property
    def github_access_token(self):
        key = "GITHUB_ACCESS_TOKEN_" + self.name
        return environ.get(key, self._github_access_token)

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:
I'd love to have your feedback on the URL_<recipe> environment variable approach combined with https://$TOKEN@github.com/$USERNAME/$REPOSITORY.git

@AndreMiras

AndreMiras commented Oct 19, 2024

Copy link
Copy Markdown
Member

Do python recipes already support getting the repository this way via 'git clone'? I only see https download support in the PythonRecipe.

git clone https://$TOKEN@github.com/$USERNAME/$REPOSITORY.git

We need this to be automated for our build process, not done by hand as a separate process.

Yes git cloning is supported in Recipe, see https://github.com/kivy/python-for-android/blob/74b891e/pythonforandroid/recipe.py#L221-L242

Regarding the hardcoded environment variable- I noticed the PythonRecipe already has a provision for using properties based on enviornment variables:

 @property
    def github_access_token(self):
        key = "GITHUB_ACCESS_TOKEN_" + self.name
        return environ.get(key, self._github_access_token)

Good catch for environment variable based properties.
https://github.com/kivy/python-for-android/blob/74b891e/pythonforandroid/recipe.py#L159-L162

    @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?

In my case, my recipe just gets the environment variable and sets it within the recipe, so it is not hard coded.

Yes I get that, that's what I meant, it's in the advantage list of your approach.

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.

Yes agree, one thing however is this is tailor made for GitHub only rather than a generic (header based) authentication.
Also I'm always trying to see if we can meet the requirements without growing the code base with many hidden features.
I'm totally not blocking the feature, just challenging it to see if there're good alternatives

@brentpicasso

Copy link
Copy Markdown
Contributor Author

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?

@AndreMiras

Copy link
Copy Markdown
Member

Yes I was thinking maybe generic header from environment variables could be an option too.
But did you give the git clone + token a try still?

@brentpicasso

Copy link
Copy Markdown
Contributor Author

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!

@brentpicasso

Copy link
Copy Markdown
Contributor Author

@AndreMiras I also found applied a tiny unrelated fix around a possible missed python 2.7 migration issue: 254440d

@AndreMiras AndreMiras left a comment

Copy link
Copy Markdown
Member

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')]

Comment thread pythonforandroid/recipe.py
AndreMiras
AndreMiras previously approved these changes Oct 21, 2024

@AndreMiras AndreMiras left a comment

Copy link
Copy Markdown
Member

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>]

@AndreMiras AndreMiras left a comment

Copy link
Copy Markdown
Member

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

Comment on lines +11 to +13
concurrency:
group: build-${{ github.ref }}
cancel-in-progress: true

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice addition thanks

@brentpicasso

Copy link
Copy Markdown
Contributor Author

@AndreMiras thanks, happy to help! Looks like all checks have passed.

@AndreMiras AndreMiras merged commit 10c5292 into kivy:develop Oct 22, 2024
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.

2 participants