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

Implement consul datasource #116

Merged
merged 2 commits into from
Jul 3, 2020

Conversation

gorexlv
Copy link
Contributor

@gorexlv gorexlv commented Mar 29, 2020

Describe what this PR does / why we need it

implement consul datasource

Does this pull request fix one issue?

Describe how you did it

adapter consul client api;
read the configuration immediately after connecting to consul server,
then start polling.

Describe how to verify it

Special notes for reviews

@gorexlv
Copy link
Contributor Author

gorexlv commented Mar 29, 2020

please pause the merge, more detail testing is in progress.

Is there any way to switch to draft PR?

@louyuting louyuting added area/data-source Issues or PRs related to data-source extension to-review PRs to review labels Mar 29, 2020
@louyuting louyuting changed the title fix #75; implement consul datasource Implement consul datasource Mar 30, 2020
@louyuting louyuting linked an issue Mar 30, 2020 that may be closed by this pull request
@louyuting
Copy link
Collaborator

please pause the merge, more detail testing is in progress.

Is there any way to switch to draft PR?

Can not.
You could create Draft PR. Once you creates OPNE PR, you can not switch to Draft.

@louyuting louyuting removed the to-review PRs to review label Mar 30, 2020
@louyuting
Copy link
Collaborator

By the way, The CI failed, could you please fix it?

Thanks.

@gorexlv gorexlv force-pushed the feat/consul-datasource branch 2 times, most recently from 16e01c2 to d00e82f Compare April 9, 2020 05:33
@louyuting louyuting self-requested a review June 14, 2020 08:11
@louyuting
Copy link
Collaborator

Could you please rebase the code?
CI shows the conflicts.
Thanks

@gorexlv gorexlv force-pushed the feat/consul-datasource branch 2 times, most recently from f19f896 to 842529e Compare June 16, 2020 14:49
@gorexlv
Copy link
Contributor Author

gorexlv commented Jun 17, 2020

#159

@codecov-commenter
Copy link

codecov-commenter commented Jun 18, 2020

Codecov Report

Merging #116 into master will increase coverage by 0.04%.
The diff coverage is 46.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #116      +/-   ##
==========================================
+ Coverage   44.62%   44.66%   +0.04%     
==========================================
  Files          76       78       +2     
  Lines        4112     4218     +106     
==========================================
+ Hits         1835     1884      +49     
- Misses       2077     2126      +49     
- Partials      200      208       +8     
Impacted Files Coverage Δ
ext/datasource/consul/consul.go 40.62% <40.62%> (ø)
ext/datasource/consul/option.go 54.76% <54.76%> (ø)

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 c37b472...90a8d99. Read the comment docs.

@sczyh30 sczyh30 self-requested a review June 18, 2020 12:54
@louyuting louyuting added this to the 0.5.0 milestone Jun 18, 2020
@louyuting louyuting self-requested a review June 19, 2020 15:37
}

func (c *consulDataSource) Close() error {
if c.cancel != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I run the test:

func Test_consulDatasource_CustomizeClient(t *testing.T) {
	client, err := api.NewClient(&api.Config{
		Address: "127.0.0.1:8500",
	})
	if err != nil {
		// todo something
	}
	ds, err := NewDatasource("property_key",
		// customize consul client
		WithConsulClient(client),
		// disable dynamic datasource watch
		WithDisableWatch(false),
		// preset property handlers
		WithPropertyHandlers(),
		// reset queryOptions, defaultQueryOptions as default
		WithQueryOptions(&api.QueryOptions{}),
	)

	if err != nil {
		// todo something
	}

	if err := ds.Initialize(); err != nil {
		// todo something
	}
	time.Sleep(time.Second * 10)

	ds.Close()

	time.Sleep(time.Second * 1000)
}

But the cancel don't work as expected.

Is there anything wrong in my test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the lastest code line time.Sleep(time.Second*1000) causes you to think that there is no cancel.

I'll improve the unit test case later.


type consulDataSource struct {
datasource.Base
*options
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems this field is useless for datasource,
Deleting this field makes more sense.

Copy link
Member

@sczyh30 sczyh30 left a comment

Choose a reason for hiding this comment

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

We may also add some logs (when the data-source has been initialized) so that users could identify which key is being watched.

// customize consul client
WithConsulClient(client),
// disable dynamic datasource watch
WithDisableWatch(true),
Copy link
Member

Choose a reason for hiding this comment

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

Maybe setting false is better here?

@sczyh30
Copy link
Member

sczyh30 commented Jun 28, 2020

@louyuting We may also need to add out-of-box demo for various data-sources in example pkg. Just xxx_example_test is not enough.

@louyuting
Copy link
Collaborator

@louyuting We may also need to add out-of-box demo for various data-sources in example pkg. Just xxx_example_test is not enough.

I think so too. We might need to add those demo into example pkg.

Copy link
Collaborator

@louyuting louyuting left a comment

Choose a reason for hiding this comment

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

Thanks for nice work
I will polish related code later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/data-source Issues or PRs related to data-source extension
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add consul as dynamic datasource
4 participants