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

Adds support to create repository from a template #1395

Closed

Conversation

isouza-daitan
Copy link
Contributor

This PR adds a new create_repo_from_template method to AuthenticatedUser and Organization that uses the new /generate endpoint of a repo. I followed the same pattern as the similar create_fork method;

I also added a new is_template attribute to the Repository class. This attribute will only be sent by Github when we use the feature's Accept header which I only added to the POST to create the new repo. Let me know if I should also add it to get_repo calls.

The documentation mentions a template_repository attribute but Github never sends it.

tests/AuthenticatedUser.py Outdated Show resolved Hide resolved
tests/Organization.py Outdated Show resolved Hide resolved
tests/Repository.py Outdated Show resolved Hide resolved
), private
post_parameters = {
"name": name,
"owner": self.login,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should use self._identity here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

neither Organization nor AuthenticatedUser have an _identity property

scripts/add_attribute.py Outdated Show resolved Hide resolved
@@ -357,6 +357,47 @@ def create_fork(self, repo):
self._requester, headers, data, completed=True
)

def create_repo_from_template(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we should abstract this out to a mix-in, since it's identical...

@s-t-e-v-e-n-k
Copy link
Collaborator

I love the direction, but I think we need to have a think about how we implement this before we land it.

@sfdye
Copy link
Member

sfdye commented Feb 13, 2020

Codecov Report

Merging #1395 into master will increase coverage by 0.14%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1395      +/-   ##
==========================================
+ Coverage   98.20%   98.35%   +0.14%     
==========================================
  Files         188      188              
  Lines       14071    14140      +69     
==========================================
+ Hits        13819    13907      +88     
+ Misses        252      233      -19     
Impacted Files Coverage Δ
tests/Framework.py 96.00% <0.00%> (-0.02%) ⬇️
tests/Logging_.py 100.00% <0.00%> (ø) ⬆️
tests/NamedUser.py 100.00% <0.00%> (ø) ⬆️
tests/GitRelease.py 100.00% <0.00%> (ø) ⬆️
tests/Repository.py 100.00% <0.00%> (ø) ⬆️
tests/Persistence.py 100.00% <0.00%> (ø) ⬆️
tests/RateLimiting.py 100.00% <0.00%> (ø) ⬆️
github/Repository.py 97.29% <0.00%> (+0.74%) ⬆️
tests/AllTests.py 100.00% <0.00%> (+1.07%) ⬆️
github/Rate.py 100.00% <0.00%> (+4.16%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5ce491...1aac6e9. Read the comment docs.

@isouza-daitan
Copy link
Contributor Author

@s-t-e-v-e-n-k thanks for the review!

The create_fork and create_repo_from_template methods are different than the other methods in the class because they are Repository endpoints, instead of Organization or User endpoints. Maybe we should move them to Repository? I think it would be harder to use but would at least match the API endpoint.

We could even keep the Org and AuthUser methods which would call the Repo method and set the appropriate owner. Thoughts?

@codecov-io
Copy link

codecov-io commented Feb 13, 2020

Codecov Report

Merging #1395 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1395   +/-   ##
=======================================
  Coverage   98.35%   98.35%           
=======================================
  Files         188      188           
  Lines       14140    14140           
=======================================
  Hits        13907    13907           
  Misses        233      233

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b4f5991...d1c282e. Read the comment docs.

@isouza-daitan
Copy link
Contributor Author

@s-t-e-v-e-n-k thoughts on my comment above?

@s-t-e-v-e-n-k
Copy link
Collaborator

@isouza-daitan Sorry for the late reply. From a correctness perspective Repository is probably the right place, but given our architecture it's hard to require a user grab a repository object just to create a different repo, potentially in a different namespace entirely.

I'm still against the code duplication -- how about one method on AuthenicatedUser that also takes owner as NotSet. If owner is NotSet, use self.login, otherwise assert it's an Organization (or an id). What do you think?

@isouza-daitan
Copy link
Contributor Author

@s-t-e-v-e-n-k won't we have auth problems if we try to create a new repo in an org using an AuthenticatedUser instance?

I agree getting a Repository to create a new repo is not great.
How about a new RepositoryActions module that has:

@staticmethod
def create_repo_from_template(
            new_repo_owner,  # AuthenticatedUser or Organization
            new_repo_name,
            template_full_name,  # my-org/my-template-name
            description=github.GithubObject.NotSet,
            private=github.GithubObject.NotSet,
    ):

and we call RepositoryActions.create_repo_from_template from AuthenticatedUser and Organization

@stale
Copy link

stale bot commented May 12, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label May 12, 2020
@stale stale bot closed this May 19, 2020
@sfdye sfdye reopened this May 19, 2020
@stale stale bot removed the stale label May 19, 2020
@codecov-commenter
Copy link

codecov-commenter commented May 19, 2020

Codecov Report

Merging #1395 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1395   +/-   ##
=======================================
  Coverage   98.67%   98.67%           
=======================================
  Files          48       48           
  Lines        2561     2562    +1     
=======================================
+ Hits         2527     2528    +1     
  Misses         34       34           
Impacted Files Coverage Δ
github/Consts.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ec4f15...a95e448. Read the comment docs.

@stale
Copy link

stale bot commented Jul 18, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jul 18, 2020
@sfdye
Copy link
Member

sfdye commented Jul 18, 2020

  1. Could you fix the python 3.6 tests?
  2. I still see travis check in the PR, can you rebase against master?

@stale stale bot removed the stale label Jul 18, 2020
@stale
Copy link

stale bot commented Sep 20, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 20, 2020
@stale stale bot removed the stale label Sep 29, 2020
@isacssouza
Copy link

@sfdye done, sorry for the delay.

@rue-nsilverman
Copy link

Its also worth adding that the include_all_branches parameter now exists on the create repo from template API. This would be hugely beneficial to add in to this function.

@simkimsia
Copy link
Contributor

I kinda need this. Can solve the conflict? @isouza-daitan

@simkimsia simkimsia mentioned this pull request Jul 31, 2021
This was referenced Oct 21, 2021
@stale
Copy link

stale bot commented Jan 8, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 8, 2022
@stale stale bot closed this Apr 16, 2022
@prein
Copy link

prein commented Aug 5, 2022

Please reopen and consider merging #2090

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants