feat: storage grafana path in to etcd #2362

jwrookie · 2022-03-07T05:34:51Z

Please answer these questions before submitting a pull request, or your PR will get closed.

Why submit this pull request?

Bugfix
New feature provided
Improve performance
Backport patches

What changes will this PR take into?
storage grafana path in to etcd

Related issues

fix/resolve #1953

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

codecov-commenter · 2022-03-07T05:36:34Z

Codecov Report

Merging #2362 (437efa6) into master (9d99ab7) will decrease coverage by 5.24%.
The diff coverage is 83.33%.

❗ Current head 437efa6 differs from pull request most recent head 0805c42. Consider uploading reports for the commit 0805c42 to get more accurate results

@@            Coverage Diff             @@
##           master    #2362      +/-   ##
==========================================
- Coverage   69.75%   64.51%   -5.25%     
==========================================
  Files         184       58     -126     
  Lines        7284     3965    -3319     
  Branches      832        0     -832     
==========================================
- Hits         5081     2558    -2523     
+ Misses       1907     1044     -863     
- Partials      296      363      +67

Flag	Coverage Δ
backend-e2e-test	`36.92% <28.33%> (-0.19%)`	⬇️
backend-e2e-test-ginkgo	`60.00% <83.33%> (+0.79%)`	⬆️
backend-unit-test	`?`
frontend-e2e-test	`?`

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

Impacted Files	Coverage Δ
api/internal/core/entity/entity.go	`90.90% <ø> (ø)`
api/internal/core/store/storehub.go	`72.91% <77.77%> (+0.32%)`	⬆️
...pi/internal/handler/system_config/system_config.go	`84.00% <84.00%> (ø)`
api/internal/route.go	`87.50% <100.00%> (+0.32%)`	⬆️
api/internal/utils/runtime/runtime.go	`5.55% <0.00%> (-61.12%)`	⬇️
api/internal/core/store/validate_mock.go	`0.00% <0.00%> (-60.00%)`	⬇️
api/internal/core/store/store.go	`65.62% <0.00%> (-23.96%)`	⬇️
api/internal/handler/service/service.go	`69.35% <0.00%> (-23.39%)`	⬇️
api/internal/filter/authentication.go	`55.55% <0.00%> (-22.23%)`	⬇️
api/internal/handler/global_rule/global_rule.go	`68.11% <0.00%> (-17.40%)`	⬇️
... and 143 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 6367ffd...0805c42. Read the comment docs.

jwrookie · 2022-03-07T06:39:45Z

Frontend e2e test error, pls rerun thx

api/internal/handler/system_config/system_config.go

api/test/e2enew/system_config/system_config_test.go

api/internal/core/entity/entity.go

api/internal/core/store/storehub.go

api/internal/handler/handler_test.go

api/internal/handler/system_config/system_config_test.go

api/internal/handler/system_config/system_config.go

starsz

I think the front-end should also support it.

api/conf/schema.json

jwrookie · 2022-03-09T03:32:42Z

Frontend e2e test error, pls rerun thx

jwrookie · 2022-03-09T09:59:54Z

@bzp2010 have a check, thx

bzp2010 · 2022-03-09T13:03:20Z

api/conf/schema.json

@@ -3022,6 +3022,34 @@
 		"upstream_hash_vars_schema": {
 			"pattern": "^((uri|server_name|server_addr|request_uri|remote_port|remote_addr|query_string|host|hostname)|arg_[0-9a-zA-z_-]+)$",
 			"type": "string"
+		},
+		"system_config":{


No need to add a schema for this, this file will only be exported by the APISIX control API. The model looks simple and we check it manually when we modify the data.

see https://github.com/apache/apisix-dashboard/pull/2362/files/f330219fd6a4975b68fd7e7a9b934f5aa062b42d#r820570419

Please keep the same opinion, I think both are fine, but according to the existing code style, it should be schema @bzp2010 @nic-chen

Please take a look at this issue, schema.json will be automatically generated by control API only, if we add it manually, it will be difficult to maintain. Do you have any suggestion?

@jwrookie I think the comment only needs to be resolved by the reviewer himself, if it is always closed by your, other reviewers will not be aware of the progress of the review and existing problems. This time I didn't notice this review. 😂

@nic-chen Please take a look at this issue, schema.json will be automatically generated by control API only, if we add it manually, it will be difficult to maintain. Do you have any suggestion?

I'm sorry, I handled it the wrong way

@bzp2010
good catch!
we could put the schema into another file, and when the Manager API starts, we need to merge its content into schema.json

I would be happy to do it by other pr

api/internal/core/store/storehub.go

bzp2010 · 2022-03-09T13:22:44Z

api/internal/handler/system_config/system_config_test.go

+	"github.com/apisix/manager-api/internal/core/store"
+)
+
+func TestSystem_Get(t *testing.T) {


In fact, we can remove these meaningless test cases, just keep only the E2E test.

I think the unit test is also required, it can quickly detect coding problems

apisix-dashboard/api/internal/handler/stream_route/stream_route_test.go

Lines 31 to 71 in 561ed37

func TestStructUnmarshal(t *testing.T) {

// define and parse data

jsonStr := `{

"id": 1,

"create_time": 1700000000,

"update_time": 1700000000,

"desc": "desc",

"remote_addr": "1.1.1.1",

"server_addr": "2.2.2.2",

"server_port": 9080,

"sni": "example.com",

"upstream": {

"nodes": [

{

"host": "10.10.10.10",

"port": 8080,

"weight": 1

}

],

"type": "roundrobin",

"scheme": "http",

"pass_host": "pass"

},

"upstream_id": 1

}`

streamRoute := entity.StreamRoute{}

err := json.Unmarshal([]byte(jsonStr), &streamRoute)

// asserts

assert.Nil(t, err)

assert.Equal(t, streamRoute.ID, float64(1))

assert.Equal(t, streamRoute.CreateTime, int64(1700000000))

assert.Equal(t, streamRoute.UpdateTime, int64(1700000000))

assert.Equal(t, streamRoute.Desc, "desc")

assert.Equal(t, streamRoute.RemoteAddr, "1.1.1.1")

assert.Equal(t, streamRoute.ServerAddr, "2.2.2.2")

assert.Equal(t, streamRoute.ServerPort, 9080)

assert.Equal(t, streamRoute.SNI, "example.com")

assert.Equal(t, streamRoute.UpstreamID, float64(1))

assert.NotNil(t, streamRoute.Upstream)

}

You can take a look at these, I don't think it makes sense to run these tests through Mock, but unit tests can be used such as data unmarshal.

Do you mean get needs to test data unmarshal, the other operations remain unchanged?

I mean, the current use cases like Get Put use mock storage to test, which doesn't make a lot of sense because mock doesn't have its own storage features like in-memory, etc (We also don't know if the handler is actually working properly, because the input and output are provided by the developer code). The storage layer that is really being used will be tested in full in E2E, in unit we just do some other trivial work.

This is my own idea, what do you think?

thx for your reply!

I think that only the data layer would use mock instead, and the unit test can walk through the handle logic, so it makes sense.

Maybe it's because the handle very simple and you think it doesn't make sense？ if that's the case, I take your advice.

😂, I think all can, please @nic-chen @starsz to see it.

Yes, most unit test cases are the same and meaningless, you could have a look at unit test cases of other handlers, they are almost the same.
We should focus on tests that are not covered.

and the unit test can walk through the handling logic, so it makes sense.

I think we can keep it, even if the logic is very simple and doesn't make sense.
Once the handle contains complex logic, the effect will be obvious.

Oh, please use JSON schema to verify, not hard code, unless it can't use JSON schema

jwrookie · 2022-03-10T04:58:45Z

@nic-chen after communicating with @bzp2010 , it will be done by other pr

jwrookie · 2022-03-10T05:01:17Z

it will be done by other pr

BTW, pls rerun the front e2e test ,thx

nic-chen · 2022-03-10T07:28:54Z

@nic-chen after communicating with @bzp2010 , it will be done by other pr

please create an issue to track it, thanks.

forgaoqiang · 2022-03-17T06:37:19Z

finally, this feature come into the reality

jwrookie added 6 commits March 4, 2022 19:16

feat: storage grafana path in to etcd

95807e5

feat: storage grafana path in to etcd

e24239f

feat: storage grafana path in to etcd

fd1d421

Merge remote-tracking branch 'upstream/master'

c524f7d

chore: ci

5213454

chore: conf.yaml

09eada0

fix: ci

f330219

nic-chen requested review from bzp2010, nic-chen and starsz March 7, 2022 07:03

starsz reviewed Mar 7, 2022

View reviewed changes

nic-chen reviewed Mar 7, 2022

View reviewed changes

fix: review

67c7c28

starsz approved these changes Mar 9, 2022

View reviewed changes

nic-chen reviewed Mar 9, 2022

View reviewed changes

api/conf/schema.json Outdated Show resolved Hide resolved

fix: review

eb48288

nic-chen previously approved these changes Mar 9, 2022

View reviewed changes

bzp2010 reviewed Mar 9, 2022

View reviewed changes

fix

0805c42

jwrookie requested a review from bzp2010 March 10, 2022 03:26

nic-chen approved these changes Mar 10, 2022

View reviewed changes

jwrookie mentioned this pull request Mar 10, 2022

TODO: model checking thougth by json schema #2372

Closed

bzp2010 approved these changes Mar 11, 2022

View reviewed changes

bzp2010 merged commit edeed6b into apache:master Mar 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: storage grafana path in to etcd #2362

feat: storage grafana path in to etcd #2362

jwrookie commented Mar 7, 2022

codecov-commenter commented Mar 7, 2022 •

edited

Loading

jwrookie commented Mar 7, 2022

starsz left a comment

jwrookie commented Mar 9, 2022

jwrookie commented Mar 9, 2022

bzp2010 Mar 9, 2022

jwrookie Mar 9, 2022

jwrookie Mar 9, 2022

bzp2010 Mar 9, 2022 •

edited

Loading

jwrookie Mar 9, 2022

nic-chen Mar 10, 2022

jwrookie Mar 10, 2022

bzp2010 Mar 9, 2022

jwrookie Mar 9, 2022

bzp2010 Mar 9, 2022

jwrookie Mar 9, 2022

bzp2010 Mar 9, 2022

jwrookie Mar 9, 2022

bzp2010 Mar 9, 2022

nic-chen Mar 10, 2022

starsz Mar 10, 2022

jwrookie commented Mar 10, 2022

jwrookie commented Mar 10, 2022

nic-chen commented Mar 10, 2022

forgaoqiang commented Mar 17, 2022

	func TestStructUnmarshal(t *testing.T) {
	// define and parse data
	jsonStr := `{
	"id": 1,
	"create_time": 1700000000,
	"update_time": 1700000000,
	"desc": "desc",
	"remote_addr": "1.1.1.1",
	"server_addr": "2.2.2.2",
	"server_port": 9080,
	"sni": "example.com",
	"upstream": {
	"nodes": [
	{
	"host": "10.10.10.10",
	"port": 8080,
	"weight": 1
	}
	],
	"type": "roundrobin",
	"scheme": "http",
	"pass_host": "pass"
	},
	"upstream_id": 1
	}`
	streamRoute := entity.StreamRoute{}
	err := json.Unmarshal([]byte(jsonStr), &streamRoute)

	// asserts
	assert.Nil(t, err)
	assert.Equal(t, streamRoute.ID, float64(1))
	assert.Equal(t, streamRoute.CreateTime, int64(1700000000))
	assert.Equal(t, streamRoute.UpdateTime, int64(1700000000))
	assert.Equal(t, streamRoute.Desc, "desc")
	assert.Equal(t, streamRoute.RemoteAddr, "1.1.1.1")
	assert.Equal(t, streamRoute.ServerAddr, "2.2.2.2")
	assert.Equal(t, streamRoute.ServerPort, 9080)
	assert.Equal(t, streamRoute.SNI, "example.com")
	assert.Equal(t, streamRoute.UpstreamID, float64(1))
	assert.NotNil(t, streamRoute.Upstream)
	}

feat: storage grafana path in to etcd #2362

feat: storage grafana path in to etcd #2362

Conversation

jwrookie commented Mar 7, 2022

codecov-commenter commented Mar 7, 2022 • edited Loading

Codecov Report

jwrookie commented Mar 7, 2022

starsz left a comment

Choose a reason for hiding this comment

jwrookie commented Mar 9, 2022

jwrookie commented Mar 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzp2010 Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwrookie commented Mar 10, 2022

jwrookie commented Mar 10, 2022

nic-chen commented Mar 10, 2022

forgaoqiang commented Mar 17, 2022

codecov-commenter commented Mar 7, 2022 •

edited

Loading

bzp2010 Mar 9, 2022 •

edited

Loading