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

#13091 - add new mirror button #13105

Merged
merged 11 commits into from
Oct 23, 2020

Conversation

divbhasin
Copy link
Contributor

This PR targets #13091. I have tested it manually, and it works. However, I am not sure if this is the best way to do it. I had some trouble on getting URL parameters to carry through to the git service-specific migrate page. Please let me know if there is a better way to get these URL parameters through. I made changes to repo/migrate/migrate.tmpl and repo/migrate.go to accomplish this. The rest is just adding the button.

Screenshots

The screenshots below document the entire flow. The URL params I attached can be seen in the address bar.

Screen Shot 2020-10-11 at 7 32 35 PM
Screen Shot 2020-10-11 at 7 36 52 PM
Screen Shot 2020-10-11 at 7 37 03 PM

@techknowlogick techknowlogick added topic/ui Change the appearance of the Gitea UI type/enhancement An improvement of existing functionality labels Oct 11, 2020
@divbhasin
Copy link
Contributor Author

Any ideas on what's going on with the CI run? I don't know how to fix the build as the error seems to be unrelated to my change.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 12, 2020
@techknowlogick
Copy link
Member

@divbhasin sometimes there are failed builds due to unrelated failures such as upstream dependencies failing, occasionally it'll be a race condition, although there has been a lot of work to get rid of those. I've restarted your build, and it'll likely pass this build, especially since the same test passed for all other DB types.

@codecov-io
Copy link

codecov-io commented Oct 12, 2020

Codecov Report

Merging #13105 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13105      +/-   ##
==========================================
+ Coverage   42.17%   42.19%   +0.01%     
==========================================
  Files         684      684              
  Lines       75546    75549       +3     
==========================================
+ Hits        31865    31878      +13     
+ Misses      38459    38449      -10     
  Partials     5222     5222              
Impacted Files Coverage Δ
routers/org/home.go 67.03% <100.00%> (+0.36%) ⬆️
routers/repo/migrate.go 42.30% <100.00%> (+0.90%) ⬆️
modules/indexer/stats/db.go 60.86% <0.00%> (-8.70%) ⬇️
modules/queue/unique_queue_disk_channel.go 53.84% <0.00%> (-1.54%) ⬇️
services/pull/pull.go 40.78% <0.00%> (ø)
routers/repo/view.go 38.11% <0.00%> (+0.64%) ⬆️
modules/process/manager.go 75.00% <0.00%> (+2.50%) ⬆️
models/unit.go 49.31% <0.00%> (+2.73%) ⬆️
modules/charset/charset.go 73.03% <0.00%> (+4.49%) ⬆️
modules/util/timer.go 85.71% <0.00%> (+42.85%) ⬆️

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 e4d9533...f34927f. Read the comment docs.

@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 Oct 12, 2020
@CirnoT
Copy link
Contributor

CirnoT commented Oct 12, 2020

Looks good although unsure whether we should distinguish the buttons somehow to avoid confusion.

@6543 6543 added this to the 1.14.0 milestone Oct 14, 2020
Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

Needs a condition to not show when repository.DISABLE_MIRRORS is enabled.

@divbhasin divbhasin requested a review from silverwind October 15, 2020 02:34
@divbhasin
Copy link
Contributor Author

@silverwind can you have a look, please? The add mirror button does not show now if the option you mentioned is enabled.

@divbhasin divbhasin requested a review from silverwind October 18, 2020 14:07
@@ -28,6 +28,9 @@
<div class="ui eleven wide column">
{{if .CanCreateOrgRepo}}
<div class="text right">
{{if not .DisabledMirrors}}
<a class="ui green button" href="{{AppSubUrl}}/repo/migrate?org={{.Org.ID}}&mirror=1">{{.i18n.Tr "new_mirror"}}</a>
Copy link
Member

@silverwind silverwind Oct 18, 2020

Choose a reason for hiding this comment

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

.Org and .Mirror can be nil, I think you need to conditionally add those params only when the are not nil to not break regular migrations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, but how can .Org be nil? This would be on the homepage of an Org so wouldn't .Org have to exist? Existing migrations from the dropdown to the top right will not be sending org and mirror params. In that case, from the repo selection page (second screenshot), org=&mirror= will be sent, which will have no impact on the form in screenshot 3.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, probably fine. I misinterpreted the serviceType == 0 part in the code. If that template only renders in that if clause it cannot become nil under regular means.

@go-gitea go-gitea deleted a comment from CirnoT Oct 18, 2020
Co-authored-by: silverwind <me@silverwind.io>
@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 Oct 23, 2020
@techknowlogick
Copy link
Member

🚀

@techknowlogick techknowlogick merged commit 9b11c3e into go-gitea:master Oct 23, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
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. topic/ui Change the appearance of the Gitea UI type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants