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: added support for -c to set the config file path #1420

Closed
wants to merge 6 commits into from

Conversation

vinayaksh42
Copy link

@vinayaksh42 vinayaksh42 commented Feb 3, 2021

Please answer these questions before submitting a pull request


New feature or improvement

  • Describe the details and related test reports.
    Added support for "-c" in the managerAPI to set the config File path

@codecov-io
Copy link

codecov-io commented Feb 3, 2021

Codecov Report

Merging #1420 (edc5bf6) into master (178ff5c) will decrease coverage by 19.68%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1420       +/-   ##
===========================================
- Coverage   67.70%   48.01%   -19.69%     
===========================================
  Files          48       39        -9     
  Lines        3031     2601      -430     
===========================================
- Hits         2052     1249      -803     
- Misses        739     1177      +438     
+ Partials      240      175       -65     
Impacted Files Coverage Δ
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/core/store/storehub.go 0.00% <0.00%> (-70.41%) ⬇️
api/internal/filter/cors.go 0.00% <0.00%> (-66.67%) ⬇️
api/internal/filter/schema.go 0.00% <0.00%> (-55.47%) ⬇️
api/internal/handler/upstream/upstream.go 28.81% <0.00%> (-54.24%) ⬇️
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 30 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 178ff5c...edc5bf6. Read the comment docs.

Copy link
Contributor

@starsz starsz left a comment

Choose a reason for hiding this comment

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

Can you add test in the /api/test/shell/cli_test.sh.

@@ -54,6 +54,7 @@ var (
SSLDefaultStatus = 1 //enable ssl by default
ImportSizeLimit = 10 * 1024 * 1024
PIDPath = "/tmp/manager-api.pid"
FilePathSet = "/conf/conf.yaml"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please take care of code style.

Copy link
Member

Choose a reason for hiding this comment

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

@vinayaksh42
You could run go fmt to format the code.

Copy link
Author

Choose a reason for hiding this comment

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

okay i will do that and make another commit to this pr

@juzhiyuan
Copy link
Member

FE Test failed and waiting for #1426

@juzhiyuan
Copy link
Member

may sync the latest codes to fix E2E failed.

@starsz
Copy link
Contributor

starsz commented Feb 5, 2021

Hi, @vinayaksh42
Maybe you should fix the conflict and sync the master branch.

@membphis
Copy link
Member

membphis commented Feb 5, 2021

same issue @nic-chen

@membphis
Copy link
Member

membphis commented Feb 5, 2021

@ vinayaksh42 need to merge the master branch

@vinayaksh42
Copy link
Author

@ vinayaksh42 need to merge the master branch

I have merged it.

@juzhiyuan
Copy link
Member

ping @nic-chen to check.

@starsz
Copy link
Contributor

starsz commented Feb 7, 2021

Oh sorry.@vinayaksh42 You need to merge the master branch again.

@nic-chen
Copy link
Member

nic-chen commented Feb 7, 2021

need test case for the change

@vinayaksh42
Copy link
Author

need test case for the change

Hey can you help me with the modification I need to do in order to add a test case for this?

@nic-chen
Copy link
Member

nic-chen commented Feb 8, 2021

@vinayaksh42
I think we could first refer to the implementation of -c and -p of nginx.

@starsz
Copy link
Contributor

starsz commented Feb 21, 2021

Hi @vinayaksh42, do you meet any problem?
I think you should merge the master branch.

@juzhiyuan
Copy link
Member

Hi @vinayaksh42, do you meet any problem?
I think you should merge the master branch.

Could you help to sync codes with this PR?

@starsz
Copy link
Contributor

starsz commented Feb 23, 2021

Hi @vinayaksh42, do you meet any problem?
I think you should merge the master branch.

Could you help to sync codes with this PR?

Sure, my pleasure.

@starsz
Copy link
Contributor

starsz commented Feb 25, 2021

I think we can merge it first.
What do you think? @nic-chen @juzhiyuan @liuxiran @imjoey

@nic-chen
Copy link
Member

we need some test cases for it.

@juzhiyuan
Copy link
Member

Hi @vinayaksh42, do you need any help from the community?

@nic-chen
Copy link
Member

ping @vinayaksh42

@vinayaksh42
Copy link
Author

Hi @vinayaksh42, do you need any help from the community?

Hey, I am not sure about how to write the test

@juzhiyuan
Copy link
Member

This may need @nic-chen @starsz 's help

@nic-chen
Copy link
Member

Not only need to add tests, but also consider the case where only -c or only -p is specified, or the two parameters are specified together.

@starsz
Copy link
Contributor

starsz commented Apr 16, 2021

Hi @vinayaksh42, do you need any help from the community?

Hey, I am not sure about how to write the test

Can you give more details about it?
So that I can help you solve it.

Maybe you can refer https://github.com/apache/apisix-dashboard/blob/master/api/test/shell/cli_test.sh.

@LiteSun
Copy link
Member

LiteSun commented May 20, 2021

ping

@vinayaksh42
Copy link
Author

ping

Really sorry for the blockage. I won't be able to contribute since I don't have my dev environment (desktop). If it's okay I would like to close this PR?

@nic-chen
Copy link
Member

ping

Really sorry for the blockage. I won't be able to contribute since I don't have my dev environment (desktop). If it's okay I would like to close this PR?

sure, thanks.

@nic-chen nic-chen closed this May 21, 2021
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.

7 participants