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

feat: support etcd prefix as apisix does #1477

Merged
merged 11 commits into from
Feb 24, 2021
Merged

Conversation

liuyang211
Copy link
Contributor

@liuyang211 liuyang211 commented Feb 17, 2021

Please answer these questions before submitting a pull request


New feature or improvement

  • Describe the details and related test reports.
    support etcd prefix as apisix does

Please add the corresponding test cases if necessary.


Backport patches

  • Why need to backport?

  • Source branch

  • Related commits and pull requests

  • Target branch

@nic-chen nic-chen changed the title support etcd prefix as apisix does feat: support etcd prefix as apisix does Feb 17, 2021
@juzhiyuan
Copy link
Member

juzhiyuan commented Feb 17, 2021

Hi, I see there have 3 checks failed:

  • Backend CLI & Backend E2E: @nic-chen will help you to check what happened.
  • Semantic Pull Request: You could ignore it safely.

@starsz
Copy link
Contributor

starsz commented Feb 18, 2021

Can you write some test cases at "/api/test/shell/cli_test.sh" to test this?

@juzhiyuan
Copy link
Member

Hi, please sync your codes with the master branch to fix the CI failure.

@codecov-io
Copy link

codecov-io commented Feb 18, 2021

Codecov Report

Merging #1477 (901b399) into master (bd5ed03) will decrease coverage by 17.32%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1477       +/-   ##
===========================================
- Coverage   68.95%   51.63%   -17.33%     
===========================================
  Files          48       39        -9     
  Lines        3038     2597      -441     
===========================================
- Hits         2095     1341      -754     
- Misses        705     1082      +377     
+ Partials      238      174       -64     
Impacted Files Coverage Δ
api/internal/core/store/storehub.go 0.00% <0.00%> (-74.49%) ⬇️
api/internal/utils/version.go 0.00% <0.00%> (-100.00%) ⬇️
api/internal/filter/request_id.go 0.00% <0.00%> (-100.00%) ⬇️
api/internal/core/entity/entity.go 0.00% <0.00%> (-100.00%) ⬇️
api/internal/filter/cors.go 0.00% <0.00%> (-66.67%) ⬇️
api/internal/filter/schema.go 0.00% <0.00%> (-55.47%) ⬇️
api/internal/utils/consts/api_error.go 0.00% <0.00%> (-50.00%) ⬇️
api/internal/handler/data_loader/route_import.go 27.41% <0.00%> (-37.50%) ⬇️
api/internal/handler/server_info/server_info.go 57.14% <0.00%> (-33.34%) ⬇️
... and 29 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 bd5ed03...901b399. Read the comment docs.

@nic-chen
Copy link
Member

@@ -225,10 +226,16 @@ func initEtcdConfig(conf Etcd) {
endpoints = conf.Endpoints
}

var prefix = "/apisix"
Copy link
Contributor

Choose a reason for hiding this comment

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

Use short assignment is better:

prefix := "/apisix"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll change it this way.

@liuyang211
Copy link
Contributor Author

liuyang211 commented Feb 20, 2021

@liuyang211 need some test cases. please refer to https://github.com/apache/apisix-dashboard/blob/master/api/test/shell/cli_test.sh#L300-L327
thanks.

I run these cmds to test it locally,

cd apisix-dashboard/api
./test/shell/cli_test.sh

but fail at
./test/shell/cli_test.sh: line 347: ./etcd-v3.4.14-linux-amd64/etcd: No such file or directory

Do I miss something?

BTW

  1. /tmp/manager-api is used as both directory and file.
    https://github.com/apache/apisix-dashboard/blob/master/api/test/shell/cli_test.sh#L143-L144
    https://github.com/apache/apisix-dashboard/blob/master/api/test/shell/cli_test.sh#L166
    This seems to raise this error if rm /tmp/manager-api is not manually excuted before next test.
    cp: cannot overwrite non-directory '/tmp/manager-api' with directory '/home/newbie/dev/apisix-dashboard/api'

  2. These two lines should swap to avoid error message when stopping manager-api
    https://github.com/apache/apisix-dashboard/blob/master/api/test/shell/cli_test.sh#L385-L387

@nic-chen
Copy link
Member

@liuyang211
please have a look at https://github.com/apache/apisix-dashboard/blob/master/.github/workflows/backend-cli-test.yml#L27-L31

If there is something not good, you could fix it. thanks.

@liuyang211
Copy link
Contributor Author

@liuyang211
please have a look at https://github.com/apache/apisix-dashboard/blob/master/.github/workflows/backend-cli-test.yml#L27-L31

If there is something not good, you could fix it. thanks.

Hi, the testcase runs OK in my local machine, but fails in the github workflow, could you review my testcase and give some advice? Thanks.

@juzhiyuan
Copy link
Member

cc @nic-chen to take a look

@nic-chen
Copy link
Member

@liuyang211
The reason for the failure is that there is no sleep after the previous stop, which causes the Manager API to not really restart successfully:
https://github.com/apache/apisix-dashboard/pull/1477/checks?check_run_id=1946779863#step:5:530

@liuyang211
Copy link
Contributor Author

liuyang211 commented Feb 23, 2021

@liuyang211
The reason for the failure is that there is no sleep after the previous stop, which causes the Manager API to not really restart successfully:
https://github.com/apache/apisix-dashboard/pull/1477/checks?check_run_id=1946779863#step:5:530

Thanks, the testcase is OK now.
I'll add more testcases for routes, services, etc.

@nic-chen nic-chen merged commit e5f09e6 into apache:master Feb 24, 2021
@nic-chen
Copy link
Member

@liuyang211
merged. thanks a lot !

@liuyang211
Copy link
Contributor Author

@liuyang211
merged. thanks a lot !

Thank you for your help!
@nic-chen @juzhiyuan @starsz @tokers

@nic-chen
Copy link
Member

@liuyang211
More contributions are welcome ! : )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: support etcd prefix as apisix does
6 participants