feat: Add Stomp (Active MQ) output plugin #7995

amus-sal · 2020-08-17T08:49:21Z

Required for all PRs:

Signed CLA.
Associated README.md updated.
Has appropriate unit tests.

ssoroka

Thanks for the new plugin! I have some change requests for you.

plugins/outputs/stomp/stomp.go

plugins/outputs/stomp/stomp_test.go

ssoroka

~~Just needs work for certificate support~~

plugins/outputs/stomp/stomp.go

ssoroka · 2020-09-10T16:22:22Z

Readme format indentation doesn't match existing examples. Still needs more tests. (minimum one green-path test that runs as -short)

amus-sal · 2020-09-27T11:04:18Z

ust needs work for certificate support

I need more info, i think I follow the implementation of the plugin "mqtt" in correct way .

amus-sal · 2020-09-27T11:11:35Z

minimum one green-path test that runs as -short)

for the test case , I have already added the min test case with green path.

plugins/outputs/stomp/stomp.go

plugins/outputs/stomp/stomp_test.go

ssoroka · 2020-10-07T16:33:05Z

Your test case is skipped when testing.Short() is true. This means most of the time zero test cases will run as part of the automated test suite, and you can't be sure that future changes won't break things.

AShabana · 2020-11-05T09:56:28Z

Hi all , Did anyone managed to solve the merge conflict ?

srebhan

Thanks @amus-sal for this plugin! I have some comments and want to ask you if there is a way to mockup the ActiveMQ side for unit-testing. It would be nice to test of metrics are sent in shorttesting mode...

docs/LICENSE_OF_DEPENDENCIES.md

go.mod

plugins/outputs/stomp/README.md

plugins/outputs/stomp/stomp.go

plugins/outputs/stomp/stomp_test.go

srebhan · 2022-02-04T16:32:20Z

@amus-sal any news here?

amus-sal · 2022-02-08T14:52:05Z

update with the required comments

amus-sal · 2022-02-08T14:52:51Z

@srebhan check now

srebhan

Thanks for the update @amus-sal! I have some more comments. Please also check the linter warnings. You can check those offline using make lint-branch and use markdownlint on your README.md file.

plugins/outputs/stomp/README.md

plugins/outputs/stomp/stomp.go

amus-sal · 2022-03-10T14:18:46Z

Thanks @srebhan, check the updated pr with your suggestions

srebhan

Nice update @amus-sal! I have some more smaller comments... Please take a look.

plugins/outputs/stomp/README.md

srebhan · 2022-03-11T10:37:51Z

plugins/outputs/stomp/stomp.go

+	Username  string          `toml:"username,omitempty"`
+	Password  string          `toml:"password,omitempty"`


Please remove the omitempty setting

Suggested change

Username string `toml:"username,omitempty"`

Password string `toml:"password,omitempty"`

Username string `toml:"username"`

Password string `toml:"password"`

I need them as there are versions of AMQ supports integration without authentication.

Well that's true, but in any case the value will be nil which means an empty string. omitempty only has an effect when serializing this struct which should never be the case actually.

plugins/outputs/stomp/stomp.go

plugins/outputs/stomp/stomp_test.go

amus-sal · 2022-03-11T11:22:54Z

Check now

amus-sal · 2022-03-15T18:20:07Z

any updates ?

amus-sal · 2022-04-11T07:40:04Z

any updates regarding my PR ?

srebhan

Hey @amus-sal, sorry the late reply and thanks for the nice update. I have three minor comments but we are very close...

plugins/outputs/stomp/stomp.go

srebhan · 2022-04-25T12:20:10Z

plugins/outputs/stomp/stomp.go

+	Username  string          `toml:"username,omitempty"`
+	Password  string          `toml:"password,omitempty"`


Well that's true, but in any case the value will be nil which means an empty string. omitempty only has an effect when serializing this struct which should never be the case actually.

plugins/outputs/stomp/stomp.go

powersj · 2022-06-17T14:00:34Z

Take a look at https://github.com/influxdata/telegraf/pull/7995/files if shows 23 files are changed. When you are only adding two originally. It does look like those other files contain changes we added recently.

amus-sal · 2022-06-17T14:23:26Z

Take a look at https://github.com/influxdata/telegraf/pull/7995/files if shows 23 files are changed. When you are only adding two originally. It does look like those other files contain changes we added recently.

I did fetch upstream with the original repo and this is the output correct me if I am wrong is this right step ?

sspaink · 2022-06-29T19:21:37Z

@amus-sal looks like there are changes in this pull request that are unrelated to your Stomp output plugin, usually rebasing vs merging your changes avoids this I think. What I recommend doing is to save your changes, copy your Stomp output plugin into a separate directory and perhaps also copy this branch just in case. Then reset your branch to master, then copy your output plugin back into the branch and force push your changes. This should essentially be a manual rebase, let me know if that makes sense or if you need more help!

srebhan · 2022-07-04T20:00:26Z

@amus-sal I think your rebase went out-of-control. I suggest you save your plugin code and reset your branch to master. Then add the plugin back in. Let me know if you need help!

amus-sal · 2022-07-04T21:26:14Z

can you check it now ?

srebhan

Looks much better! Thanks for the update @amus-sal.

sspaink · 2022-07-06T19:07:46Z

go.mod

@@ -406,3 +406,5 @@ require (
 	sigs.k8s.io/structured-merge-diff/v4 v4.2.1 // indirect
 	sigs.k8s.io/yaml v1.2.0 // indirect
 )
+
+require github.com/go-stomp/stomp v2.1.4+incompatible // indirect


The linter is failing and I think its because of this, when I locally run go mod tidy it removes the // indirect comment and that seems to fix it. Could you also move this to be inside the require block instead of on its own? Locally when I run go mod tidy it puts it in line 65

but indirect related to the state if using the lib inside the module but I did an update for it but still exist, I think the problems that the plugin path "o export data for "github.com/influxdata/telegraf/plugins/outputs/stomp"\n\n" "is not yet pushed to remote repo. btw I run Golangci-lint locally and it works well with both

srebhan · 2022-07-07T14:33:59Z

@amus-sal did you run make tidy before pushing?

amus-sal · 2022-07-08T08:00:08Z

yes I did and no changes found

amus-sal · 2022-07-11T08:07:11Z

any help in that ?

srebhan · 2022-07-11T12:20:28Z

@amus-sal I see you referenced a sample.conf file (the one that is embedded into your plugin code), but there is none in your PR. Did you maybe forget to check that in?

amus-sal · 2022-07-13T14:02:27Z

I have added the sample confirmation and did make tidy and the error of lint plugin is related to other input plugins, can you check please ?

powersj · 2022-07-13T19:13:45Z

Hi @amus-sal,

Thanks for all the work on this PR! We are nearly there.

@sspaink got tests running, and it looks like there is a missing license now in the docs/LICENSE_OF_DEPENDENCIES.md file because of pulling in go-stomp/stomp as a direct dependency:

@@ -108,0 +109 @@
+github.com/go-stomp/stomp [Apache License 2.0](https://github.com/go-stomp/stomp/blob/master/LICENSE.txt)

Can you add that package and specify it uses the Apache License 2.0 license?

Thanks!

powersj · 2022-07-14T13:14:29Z

Thanks for all the work on this!

ssoroka added the new plugin label Aug 17, 2020

ssoroka suggested changes Aug 17, 2020

View reviewed changes

amus-sal requested a review from ssoroka August 19, 2020 09:35

ssoroka suggested changes Aug 19, 2020

View reviewed changes

plugins/outputs/stomp/stomp.go Outdated Show resolved Hide resolved

amus-sal requested a review from ssoroka September 9, 2020 11:33

ssoroka suggested changes Oct 7, 2020

View reviewed changes

plugins/outputs/stomp/stomp.go Outdated Show resolved Hide resolved

plugins/outputs/stomp/stomp.go Outdated Show resolved Hide resolved

plugins/outputs/stomp/stomp_test.go Show resolved Hide resolved

sjwang90 added the plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins label Jan 22, 2021

amus-sal changed the title ~~Add Stomp (Active MQ) output plugin~~ featAdd Stomp (Active MQ) output plugin Dec 8, 2021

amus-sal changed the title ~~featAdd Stomp (Active MQ) output plugin~~ Feat: Add Stomp (Active MQ) output plugin Dec 8, 2021

amus-sal changed the title ~~Feat: Add Stomp (Active MQ) output plugin~~ feat: Add Stomp (Active MQ) output plugin Dec 8, 2021

srebhan reviewed Dec 20, 2021

View reviewed changes

srebhan self-assigned this Dec 20, 2021

amus-sal requested a review from srebhan February 8, 2022 14:52

srebhan reviewed Feb 16, 2022

View reviewed changes

amus-sal requested a review from srebhan March 10, 2022 14:18

srebhan reviewed Mar 11, 2022

View reviewed changes

amus-sal requested a review from srebhan March 11, 2022 11:23

srebhan reviewed Apr 25, 2022

View reviewed changes

amus-sal closed this Jul 4, 2022

amus-sal force-pushed the master branch from bd3ed7b to 379b8ea Compare July 4, 2022 20:50

amus-sal added 3 commits July 4, 2022 22:54

add stomp plugin

14d35af

add stomp plugin

694e894

Merge branch 'master' of https://github.com/amus-sal/telegraf

45b11f7

amus-sal reopened this Jul 4, 2022

srebhan approved these changes Jul 5, 2022

View reviewed changes

srebhan requested a review from powersj July 5, 2022 14:51

sspaink suggested changes Jul 6, 2022

View reviewed changes

amus-sal added 2 commits July 7, 2022 09:59

update go mod for stomp file

9d3e05b

update stomp lin in go mod

6948f7b

add sample conf

ab53ee6

amus-sal added 3 commits July 14, 2022 09:15

add stomp license

e9fefd5

add blanck line

f9032dd

add single newline for license file

d82f322

powersj approved these changes Jul 14, 2022

View reviewed changes

powersj merged commit b0819ba into influxdata:master Jul 14, 2022

sjwang90 mentioned this pull request Sep 7, 2022

Telegraf 1.24 influxdata/docs-v2#4422

Closed

		Username string `toml:"username,omitempty"`
		Password string `toml:"password,omitempty"`

feat: Add Stomp (Active MQ) output plugin #7995

feat: Add Stomp (Active MQ) output plugin #7995

Conversation

amus-sal commented Aug 17, 2020 • edited by sspaink Loading

Required for all PRs:

ssoroka left a comment

Choose a reason for hiding this comment

ssoroka left a comment • edited Loading

Choose a reason for hiding this comment

ssoroka commented Sep 10, 2020 • edited Loading

amus-sal commented Sep 27, 2020

amus-sal commented Sep 27, 2020

ssoroka commented Oct 7, 2020

AShabana commented Nov 5, 2020 • edited Loading

srebhan left a comment

Choose a reason for hiding this comment

srebhan commented Feb 4, 2022

amus-sal commented Feb 8, 2022

amus-sal commented Feb 8, 2022

srebhan left a comment

Choose a reason for hiding this comment

amus-sal commented Mar 10, 2022

srebhan left a comment

Choose a reason for hiding this comment

srebhan Mar 11, 2022

Choose a reason for hiding this comment

amus-sal Mar 11, 2022

Choose a reason for hiding this comment

srebhan Apr 25, 2022

Choose a reason for hiding this comment

amus-sal commented Mar 11, 2022

amus-sal commented Mar 15, 2022

amus-sal commented Apr 11, 2022

srebhan left a comment

Choose a reason for hiding this comment

srebhan Apr 25, 2022

Choose a reason for hiding this comment

powersj commented Jun 17, 2022

amus-sal commented Jun 17, 2022

sspaink commented Jun 29, 2022

srebhan commented Jul 4, 2022

amus-sal commented Jul 4, 2022

srebhan left a comment

Choose a reason for hiding this comment

sspaink Jul 6, 2022

Choose a reason for hiding this comment

amus-sal Jul 7, 2022

Choose a reason for hiding this comment

srebhan commented Jul 7, 2022

amus-sal commented Jul 8, 2022

amus-sal commented Jul 11, 2022

srebhan commented Jul 11, 2022

amus-sal commented Jul 13, 2022

powersj commented Jul 13, 2022

powersj commented Jul 14, 2022 • edited Loading

amus-sal commented Aug 17, 2020 •

edited by sspaink

Loading

ssoroka left a comment •

edited

Loading

ssoroka commented Sep 10, 2020 •

edited

Loading

AShabana commented Nov 5, 2020 •

edited

Loading

powersj commented Jul 14, 2022 •

edited

Loading