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

Set the default branch for repositories generated from templates #19136

Merged
merged 15 commits into from
Mar 27, 2022

Conversation

abheekda1
Copy link
Contributor

Should fix #19082

The generate repository function generates a repository from a template, but when used from the API, it does not seem to have any value returned for the default branch. Seeing as the API was simply returning the generated repository object converted to JSON using a function (which did indeed have the default branch filled out), it seemed likely that the default branch was just not being set for the generate repository. This was fixed simply enough (hopefully correctly) with this PR. Please let me know if I missed something, did something wrong, or anything else :)

@abheekda1
Copy link
Contributor Author

Not sure if the most recent commit will work, if anyone has any suggestions please let me know how to best approach making this simple change functional.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 19, 2022
@jolheiser
Copy link
Member

Will review when I get to my machine, but the CI is failing because you need to make generate-swagger

@abheekda1
Copy link
Contributor Author

abheekda1 commented Mar 19, 2022

Yep, thanks :)

Oh wait I did it manually based on the diff in the Drone console, hopefully it works just fine 👀

Ok it didn't, I'm just gonna run the command now. I think it was some weird formatting stuff with Vim.

@abheekda1 abheekda1 force-pushed the repogen-default-branch branch from 2010f3e to 3fbbbe0 Compare March 19, 2022 01:14
Copy link
Member

@jolheiser jolheiser left a comment

Choose a reason for hiding this comment

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

Firstly, this should also be added to the UI, as currently it only uses default branch.

Second, the current changes are only setting the default in the database, however they are not being passed along to any of the git commands when the repo is initialized.
The API response gives the input default branch, but the actual repository doesn't reflect it.

Particularly, if you follow

if err = repo_module.GenerateGitContent(ctx, templateRepo, generateRepo); err != nil {
you should run into spots that are currently using default branch that will need to change.

@abheekda1
Copy link
Contributor Author

abheekda1 commented Mar 19, 2022

repo.DefaultBranch = templateRepo.DefaultBranch

It seems this is where the default branch for the generated repo is being defined. A request missing the key for default_branch would just fill in the object with an empty string for DefaultBranch right? So would it make sense to check whether repo.DefaultBranch is an empty string and only define the default branch using the template repo if it is?

Something like:

if strings.TrimSpace(repo.DefaultBranch) == "" {
    repo.DefaultBranch = templateRepo.DefaultBranch
}

@abheekda1 abheekda1 requested a review from jolheiser March 19, 2022 04:22
@abheekda1
Copy link
Contributor Author

I think once the API is figured out the UI can be implemented as well.

@jolheiser
Copy link
Member

Default branch is also used here

func initRepoCommit(ctx context.Context, tmpPath string, repo *repo_model.Repository, u *user_model.User, defaultBranch string) (err error) {

@abheekda1
Copy link
Contributor Author

@jolheiser think that should also be implemented in a similar way as of the latest commit.

@abheekda1 abheekda1 force-pushed the repogen-default-branch branch from e854af1 to e5f19d0 Compare March 20, 2022 06:04
@Gusted Gusted added this to the 1.17.0 milestone Mar 20, 2022
@abheekda1
Copy link
Contributor Author

Hey @jolheiser sorry to bother you but I think I got it now, would you mind taking a look?

@jolheiser
Copy link
Member

It looks like this is working via API now.

@abheekda1
Copy link
Contributor Author

Is there anything left for me to do or is this PR mostly set?

Copy link
Member

@jolheiser jolheiser left a comment

Choose a reason for hiding this comment

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

I would like to see this available in the UI for parity, but I suppose that can be a separate PR since this one also fixes a bug.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 24, 2022
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 24, 2022
@codecov-commenter
Copy link

codecov-commenter commented Mar 24, 2022

Codecov Report

Merging #19136 (ef47540) into main (49c5fc5) will increase coverage by 0.00%.
The diff coverage is 38.60%.

❗ Current head ef47540 differs from pull request most recent head 6446cd3. Consider uploading reports for the commit 6446cd3 to get more accurate results

@@           Coverage Diff            @@
##             main   #19136    +/-   ##
========================================
  Coverage   46.55%   46.56%            
========================================
  Files         856      857     +1     
  Lines      123018   123341   +323     
========================================
+ Hits        57277    57428   +151     
- Misses      58814    58967   +153     
- Partials     6927     6946    +19     
Impacted Files Coverage Δ
cmd/admin.go 0.00% <0.00%> (ø)
cmd/hook.go 7.11% <0.00%> (ø)
cmd/serv.go 2.39% <0.00%> (ø)
cmd/web_acme.go 0.00% <0.00%> (ø)
models/asymkey/ssh_key_deploy.go 55.55% <ø> (+1.13%) ⬆️
models/helper_environment.go 100.00% <ø> (ø)
models/repo_generate.go 21.05% <ø> (ø)
modules/context/permission.go 25.39% <0.00%> (ø)
modules/doctor/checkOldArchives.go 22.85% <0.00%> (ø)
modules/doctor/fix16961.go 35.06% <0.00%> (ø)
... and 187 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 5fe764b...6446cd3. Read the comment docs.

@abheekda1
Copy link
Contributor Author

Any clue why this is failing CI?

@zeripath
Copy link
Contributor

Any clue why this is failing CI?

it's not your fault - it's this TestRepoIndexer. The test is inherently racey and things have now started to become super racey there. I've added #19225 which should resolve the issue.

@abheekda1
Copy link
Contributor Author

Alright, nice! Seems like it finally passed this time.

@zeripath zeripath merged commit f316582 into go-gitea:main Mar 27, 2022
@abheekda1 abheekda1 deleted the repogen-default-branch branch March 27, 2022 03:22
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
…gitea#19136)

* Set the default branch for repositories generated from templates
* Allows default branch to be set through the API for repos generated from templates
* Update swagger API template
* Only set default branch to the one from the template if not specified
* Use specified default branch if it exists while generating git commits

Fix go-gitea#19082 

Co-authored-by: John Olheiser <[email protected]>
Co-authored-by: zeripath <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When generating repo from template, default_branch returns as blank string
7 participants