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.

Sign up for GitHub

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

Jump to bottom

Refactor mirror command #860

Merged
merged 44 commits into from
Nov 23, 2020
Merged

Refactor mirror command #860

merged 44 commits into from
Nov 23, 2020

Conversation

lucklove
Copy link
Member

@lucklove lucklove commented Oct 22, 2020 •
edited
Loading

What problem does this PR solve?

#814

What is changed and how it works?

  • Support publish to local mirror
  • Support merge mirror
  • Refactor tiup-server
  • Support add owner
  • Add test

Check List

Tests

  • Unit test
  • Integration test

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation

Release notes:

Support merge offline packages

@lucklove lucklove marked this pull request as draft October 22, 2020 10:33
@codecov-io
Copy link

codecov-io commented Oct 22, 2020 •
edited
Loading

Codecov Report

Merging #860 (986fe85) into master (d1c4866) will decrease coverage by 1.75%.
The diff coverage is 52.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #860      +/-   ##
==========================================
- Coverage   53.16%   51.40%   -1.76%     
==========================================
  Files         260      261       +1     
  Lines       18956    19254     +298     
==========================================
- Hits        10078     9898     -180     
- Misses       7338     7743     +405     
- Partials     1540     1613      +73
Flag Coverage Δ
cluster 38.26% <1.12%> (-6.96%) ⬇️
dm 24.11% <1.12%> (-0.98%) ⬇️
integrate 46.23% <49.23%> (-1.53%) ⬇️
playground 20.23% <0.80%> (-1.87%) ⬇️
tiup 17.17% <49.03%> (+6.49%) ⬆️
unittest 21.64% <30.96%> (+0.12%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/env.go 92.30% <ø> (ø)
pkg/repository/store/sync.go 25.00% <0.00%> (ø)
pkg/repository/v1manifest/key_store.go 57.69% <0.00%> (-2.31%) ⬇️
pkg/repository/v1manifest/keys.go 60.00% <0.00%> (+0.98%) ⬆️
pkg/repository/v1manifest/types.go 76.92% <0.00%> (-4.16%) ⬇️
pkg/repository/mirror.go 44.95% <22.78%> (+1.38%) ⬆️
pkg/repository/v1manifest/repo.go 41.72% <26.08%> (+41.72%) ⬆️
pkg/utils/ioutil.go 50.45% <33.33%> (-3.92%) ⬇️
cmd/mirror.go 53.86% <53.79%> (+17.01%) ⬆️
pkg/repository/model/model.go 57.34% <57.34%> (ø)
... and 60 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 d1c4866...986fe85. Read the comment docs.

@lucklove lucklove changed the title Support offline package merge Refactor mirror command Oct 29, 2020
@lucklove lucklove force-pushed the merge branch 5 times, most recently from 1590f3a to 7ced4be Compare November 4, 2020 03:02
@lucklove lucklove marked this pull request as ready for review November 5, 2020 03:27
AstroProfundis
AstroProfundis reviewed Nov 13, 2020
View reviewed changes
Copy link
Contributor

@AstroProfundis AstroProfundis left a comment

Choose a reason for hiding this comment

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

And don't change the file mode of pkg/cluster/executor/executor.go

cmd/mirror.go Show resolved Hide resolved
cmd/mirror.go Outdated Show resolved Hide resolved
cmd/mirror.go Outdated Show resolved Hide resolved
pkg/cluster/audit/audit.go Show resolved Hide resolved
pkg/repository/merge_mirror.go Outdated Show resolved Hide resolved
pkg/repository/merge_mirror.go Outdated Show resolved Hide resolved
pkg/repository/merge_mirror.go Show resolved Hide resolved
pkg/repository/merge_mirror.go Show resolved Hide resolved
pkg/repository/merge_mirror.go Outdated Show resolved Hide resolved
AstroProfundis
AstroProfundis reviewed Nov 17, 2020
View reviewed changes
Copy link
Contributor

@AstroProfundis AstroProfundis left a comment

Choose a reason for hiding this comment

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

We should add more comments in txn.go to describe how the merging process work.

pkg/repository/mirror.go
}
defer resp.Body.Close()

if resp.StatusCode >= 300 {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about 301, 302, 303 and 307?

Copy link
Member Author

@lucklove lucklove Nov 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It's unexpected, the server shouldn't return this, so we just report an error

pkg/repository/mirror.go Outdated Show resolved Hide resolved
pkg/repository/mirror.go Outdated Show resolved Hide resolved
pkg/repository/mirror.go Outdated Show resolved Hide resolved
pkg/repository/mirror.go Show resolved Hide resolved
pkg/repository/model/model.go Outdated Show resolved Hide resolved
pkg/repository/store/txn.go Outdated Show resolved Hide resolved
lucklove and others added 2 commits November 17, 2020 17:21
@AstroProfundis AstroProfundis mentioned this pull request Nov 19, 2020
Closed
@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 23, 2020
@AstroProfundis
Copy link
Contributor

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 23, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit fa1a80b into pingcap:master Nov 23, 2020
@lucklove lucklove mentioned this pull request Nov 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lonng lonng lonng left review comments

@AstroProfundis AstroProfundis AstroProfundis approved these changes

Assignees
No one assigned
Labels
status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1. status/need-doc Indicates that we should update document before merge a PR. type/new-feature Categorizes pr as related to a new feature.
Projects
None yet
Milestone
v1.3.0
Development

Successfully merging this pull request may close these issues.
5 participants
@lucklove @codecov-io @AstroProfundis @ti-srebot @lonng