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

feat: add api of config migrate, export and import #1893

Merged
merged 15 commits into from
Jun 8, 2021
Merged

feat: add api of config migrate, export and import #1893

merged 15 commits into from
Jun 8, 2021

Conversation

fregie
Copy link
Contributor

@fregie fregie commented May 12, 2021 •
edited
Loading

Why submit this pull request?

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches

What changes will this PR take into?
Add backend api of export and import config.For migrate

Related issues
resolve #782

Checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

Hi everyone

I have completed the development of this feature,but I haven't add unit test or e2e test case yet,so do not merge for now.
This feature is strongly coupled with store module and core logic,so I think few functions can be unit tested.
Can I just skip unit test and directly add e2e test cases?
@nic-chen @starsz

@netlify
Copy link

netlify bot commented May 12, 2021 •
edited
Loading

✔️ Deploy Preview for apisix-dashboard ready!

🔨 Explore the source changes: f44b79b

🔍 Inspect the deploy log: https://app.netlify.com/sites/apisix-dashboard/deploys/60bd99247af1710007b1614b

😎 Browse the preview: https://deploy-preview-1893--apisix-dashboard.netlify.app

@juzhiyuan juzhiyuan requested review from nic-chen and starsz and removed request for nic-chen May 13, 2021 01:50
@codecov-commenter
Copy link

codecov-commenter commented May 13, 2021 •
edited
Loading

Codecov Report

Merging #1893 (f44b79b) into master (ee1685d) will increase coverage by 0.03%.
The diff coverage is 62.13%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1893      +/-   ##
==========================================
+ Coverage   66.94%   66.98%   +0.03%     
==========================================
  Files         169      173       +4     
  Lines        6317     6487     +170     
  Branches      748      749       +1     
==========================================
+ Hits         4229     4345     +116     
- Misses       1836     1875      +39     
- Partials      252      267      +15
Flag Coverage Δ
backend-e2e-test 44.48% <2.36%> (-2.10%) ⬇️
backend-e2e-test-ginkgo 46.68% <62.13%> (+0.87%) ⬆️
backend-unit-test 52.30% <0.00%> (-0.12%) ⬇️
frontend-e2e-test 64.24% <ø> (+0.17%) ⬆️

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

Impacted Files Coverage Δ
api/internal/core/store/storehub.go 70.00% <33.33%> (-1.03%) ⬇️
api/internal/core/migrate/dataset.go 52.72% <52.72%> (ø)
api/internal/handler/migrate/migrate.go 57.40% <57.40%> (ø)
api/internal/core/migrate/migrate.go 75.00% <75.00%> (ø)
api/internal/core/migrate/conflict.go 76.92% <76.92%> (ø)
api/internal/core/store/store.go 87.71% <100.00%> (+0.21%) ⬆️
api/internal/route.go 85.29% <100.00%> (+0.44%) ⬆️
web/src/pages/Route/List.tsx 84.52% <0.00%> (ø)
web/src/pages/Route/components/Step1/MetaView.tsx 100.00% <0.00%> (ø)
... and 7 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 ee1685d...f44b79b. Read the comment docs.

@nic-chen
Copy link
Member

Oh, big feature. thanks for contribution.
I think we should want to discuss clearly.
Do you mind sending a proposal to the maillist([email protected]) for discussion first? Thanks.

@fregie fregie force-pushed the master branch 2 times, most recently from 0780e86 to 87b458d Compare May 14, 2021 13:18
@juzhiyuan juzhiyuan closed this May 17, 2021
@juzhiyuan juzhiyuan reopened this May 17, 2021
@fregie
Copy link
Contributor Author

fregie commented May 17, 2021

@nic-chen
any update of this feature?

@nic-chen
Copy link
Member

@nic-chen
any update of this feature?

I replied on the mailing list. Please have a look.

@fregie
Copy link
Contributor Author

fregie commented May 25, 2021

@nic-chen @starsz
any update again?

@starsz
Copy link
Contributor

starsz commented May 25, 2021

@nic-chen @starsz
any update again?

Sorry for the late reply. Let me check the email and give my opinion.

@nic-chen
Copy link
Member

@nic-chen @starsz
any update again?

replied on the mailing list. Please have a look.

@moonming
Copy link
Member

A link to the mailing list needs to be given. Other people don't know what you are discussing

@nic-chen @starsz

@nic-chen
Copy link
Member

discussing maillist:

https://lists.apache.org/thread.html/r6f5036941c98ae75c4ff47361aaa2a0cf889ef90e5f7d524f34fa45c%40%3Cdev.apisix.apache.org%3E

https://lists.apache.org/thread.html/r8eb4dba77f331230e9b9bec6cf0c35b79f354ede601e741ebf20d70f%40%3Cdev.apisix.apache.org%3E

@fregie
Copy link
Contributor Author

fregie commented May 31, 2021

Can we merge now？
@starsz @nic-chen

@nic-chen
Copy link
Member

@fregie CI failed

@fregie fregie requested a review from starsz June 1, 2021 14:42
@fregie
Copy link
Contributor Author

fregie commented Jun 1, 2021

Resolved,please check it.
@starsz

starsz
starsz reviewed Jun 2, 2021
View reviewed changes
api/internal/core/migrate/allData.go Outdated Show resolved Hide resolved
api/internal/core/migrate/allData.go Outdated Show resolved Hide resolved
api/internal/core/migrate/allData.go Outdated Show resolved Hide resolved
api/internal/core/migrate/allData.go Outdated Show resolved Hide resolved
api/internal/core/migrate/migrate.go Show resolved Hide resolved
api/internal/core/migrate/migrate.go Outdated Show resolved Hide resolved
api/internal/handler/config_migrate/migrate.go Outdated Show resolved Hide resolved
@fregie
Copy link
Contributor Author

fregie commented Jun 3, 2021

Resolved,please check it.
@starsz

gxthrj
gxthrj approved these changes Jun 5, 2021
View reviewed changes
Copy link
Contributor

@gxthrj gxthrj left a comment

Choose a reason for hiding this comment

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

LGTM

@fregie fregie requested review from starsz and nic-chen June 6, 2021 14:03
starsz
starsz approved these changes Jun 6, 2021
View reviewed changes
api/internal/core/migrate/conflict.go Outdated Show resolved Hide resolved
iamayushdas
iamayushdas approved these changes Jun 7, 2021
View reviewed changes
Copy link
Contributor

@iamayushdas iamayushdas left a comment

Choose a reason for hiding this comment

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

LGTM

@fregie fregie requested a review from nic-chen June 7, 2021 04:00
imjoey
imjoey requested changes Jun 8, 2021
View reviewed changes
Copy link
Member

@imjoey imjoey left a comment

Choose a reason for hiding this comment

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

@fregie absolutely a nice PR. Thank you very much.

api/internal/core/migrate/migrate.go Show resolved Hide resolved
docs/en/latest/api/api.md Show resolved Hide resolved
docs/en/latest/api/api.md Show resolved Hide resolved
imjoey
imjoey approved these changes Jun 8, 2021
View reviewed changes
Copy link
Member

@imjoey imjoey left a comment

Choose a reason for hiding this comment

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

@fregie looks awesome to me. Thanks.

@imjoey imjoey merged commit a3928d8 into apache:master Jun 8, 2021
@nic-chen
Copy link
Member

nic-chen commented Jun 8, 2021

Thanks @fregie

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iamayushdas iamayushdas iamayushdas approved these changes

@nic-chen nic-chen nic-chen approved these changes

@starsz starsz starsz approved these changes

@gxthrj gxthrj gxthrj approved these changes

@imjoey imjoey imjoey approved these changes

@Jaycean Jaycean Awaiting requested review from Jaycean

@tokers tokers Awaiting requested review from tokers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

能否增加配置导入导出的功能，用于升级或多数据中心的apisix之间进行迁移
9 participants
@fregie @codecov-commenter @nic-chen @starsz @moonming @imjoey @gxthrj @iamayushdas @juzhiyuan