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

Git migration UX improvements #12619

Merged
merged 10 commits into from
Aug 28, 2020
Merged

Git migration UX improvements #12619

merged 10 commits into from
Aug 28, 2020

Conversation

jolheiser
Copy link
Member

@jolheiser jolheiser commented Aug 27, 2020

This PR aims to fix some bugs and improve overall migration UX.

Closes #9096

Bugs

  1. GitHub token authentication had a bug where private repositories would not get their release assets correctly because the code was trying to retrieve them without auth.
  2. GitLab's API wants a very specific format for their repo name, which would probably fail more often than not.
    • While munging the clone address we retrieved the "project path", however it retained .git at the end which would cause the client to fail

If you would like me to split out the bugs into a separate patch, I could probably take out the GitLab one. The GH one, however, mostly relies on other changes in this PR to get the downloader inside the uploader for retrieving assets with auth.

Enhancements

  1. No more "username is a token" magic
    • If the git service supports a token, the UI will disable basic auth fields and handle it appropriately on the backend
  2. Migration options are always shown, however they are disabled if no authentication is provided
  3. Authentication remains optional for all service types, however just as before items will not be available

Overall I hope the UI is a little clearer as to what is expected from the user.

This PR was tested with a private repository on both GitHub/GitLab, and was able to fully migrate everything in both cases.
I was also able to do a plain migration from private repositories on BitBucket as well as Gitea.

UI Example

Screenshot from 2020-08-26 22-41-17

Signed-off-by: jolheiser <john.olheiser@gmail.com>
Signed-off-by: jolheiser <john.olheiser@gmail.com>
Signed-off-by: jolheiser <john.olheiser@gmail.com>
Signed-off-by: jolheiser <john.olheiser@gmail.com>
@jolheiser jolheiser added type/enhancement An improvement of existing functionality topic/ui Change the appearance of the Gitea UI labels Aug 27, 2020
Signed-off-by: jolheiser <john.olheiser@gmail.com>
Signed-off-by: jolheiser <john.olheiser@gmail.com>
Signed-off-by: jolheiser <john.olheiser@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Aug 27, 2020

Codecov Report

Merging #12619 into master will increase coverage by 0.06%.
The diff coverage is 44.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12619      +/-   ##
==========================================
+ Coverage   43.39%   43.45%   +0.06%     
==========================================
  Files         645      645              
  Lines       71288    71304      +16     
==========================================
+ Hits        30932    30986      +54     
+ Misses      35340    35310      -30     
+ Partials     5016     5008       -8     
Impacted Files Coverage Δ
modules/auth/repo_form.go 42.34% <ø> (ø)
modules/migrations/base/downloader.go 23.36% <ø> (ø)
modules/migrations/git.go 46.66% <0.00%> (-3.34%) ⬇️
modules/migrations/gitea.go 7.07% <0.00%> (+0.35%) ⬆️
modules/migrations/gitlab.go 1.14% <0.00%> (-0.59%) ⬇️
modules/migrations/github.go 83.77% <61.90%> (+2.92%) ⬆️
modules/migrations/migrate.go 24.40% <66.66%> (+2.56%) ⬆️
modules/structs/repo.go 65.51% <72.22%> (+10.97%) ⬆️
routers/repo/repo.go 32.71% <100.00%> (+0.17%) ⬆️
modules/util/timer.go 42.85% <0.00%> (-42.86%) ⬇️
... and 15 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 ed2f6e1...f092106. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 27, 2020
Signed-off-by: jolheiser <john.olheiser@gmail.com>
@lunny
Copy link
Member

lunny commented Aug 27, 2020

Could you consider my propose on #9096 (comment)

@lafriks lafriks added this to the 1.13.0 milestone Aug 27, 2020
@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 Aug 27, 2020
Copy link
Member

@mrsdizzie mrsdizzie left a comment

Choose a reason for hiding this comment

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

This looks great as a new base! Probably need to reuse some of this code for the mirror settings in a repo as well which currently have the same username/password issue

@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 Aug 27, 2020
@lafriks
Copy link
Member

lafriks commented Aug 27, 2020

🚀

Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

🎉

sidenote: https://try.gitea.io/api/swagger#/repository/repoMigrate has to adapt this if it got merged

@lunny lunny merged commit 211321f into go-gitea:master Aug 28, 2020
@lafriks lafriks changed the title Git migration UX Git migration UX improvements Aug 28, 2020
@6543
Copy link
Member

6543 commented Aug 28, 2020

had a look how this afect the API ... and the only downside I noticed is that the GitServiceType is an integer
to change this to a string would require just too mouch ... the beter way is to document it and warp it on the Client sides

so the answer of this pull is: https://gitea.com/gitea/go-sdk/pulls/392

@lafriks
Copy link
Member

lafriks commented Aug 28, 2020

@6543 it can be made as enum in API but string could also be easily done imho

@6543
Copy link
Member

6543 commented Aug 28, 2020

@lafriks if we do so, we should cange it with in v1.13 to not inteoduce a api break

@jolheiser jolheiser deleted the migrate branch August 28, 2020 13:24
@lafriks
Copy link
Member

lafriks commented Aug 30, 2020

@6543 yes, that's right

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.

7 participants