Skip to content
This repository has been archived by the owner on May 30, 2022. It is now read-only.

Make all discoveries able to publish data to the Collector #371

Conversation

nelsonkopliku
Copy link
Member

@nelsonkopliku nelsonkopliku commented Oct 22, 2021

Previously we made the #361 which makes the cluster discovery able to publish data to the collector.

This PR is following up that path and it makes all the discovery loops able to publish data to the collector.

I added some tests as well, except for the sapsystem discovery, which is not really handy to test.

@nelsonkopliku nelsonkopliku force-pushed the make_all_discoveries_able_to_publish_data branch 6 times, most recently from 21c92db to fab717b Compare October 26, 2021 10:40
@nelsonkopliku nelsonkopliku marked this pull request as ready for review October 26, 2021 10:47
@nelsonkopliku nelsonkopliku force-pushed the make_all_discoveries_able_to_publish_data branch from fab717b to 7208e26 Compare October 26, 2021 11:08
@nelsonkopliku nelsonkopliku force-pushed the make_all_discoveries_able_to_publish_data branch from 7208e26 to 097b0c9 Compare October 26, 2021 12:28
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Good job!
As a follow up work, we would need to unify the json annotations to use always the same format, and not mix, camel case, underscores, capital letters, etc
I know that you just inherited this, but we should do it at some point in the future

PD: I see a lot of fixtures. We had some of them before already. Please, have a look if any of them got deprecated and we should remove the file

agent/collector/client.go Outdated Show resolved Hide resolved
@nelsonkopliku nelsonkopliku force-pushed the make_all_discoveries_able_to_publish_data branch from 097b0c9 to e03748b Compare October 27, 2021 07:03
@nelsonkopliku
Copy link
Member Author

Good job! As a follow up work, we would need to unify the json annotations to use always the same format, and not mix, camel case, underscores, capital letters, etc I know that you just inherited this, but we should do it at some point in the future

Thanks @arbulu89 for the review and feedback.
I completely agree on unifying the json outputs. Created an issue for that #385

PD: I see a lot of fixtures. We had some of them before already. Please, have a look if any of them got deprecated and we should remove the file

I think I didn't see the fixtures you're referring to 😅 . Where should I look for those?

@nelsonkopliku nelsonkopliku merged commit 423482a into trento-project:main Oct 27, 2021
@arbulu89
Copy link
Contributor

Good job! As a follow up work, we would need to unify the json annotations to use always the same format, and not mix, camel case, underscores, capital letters, etc I know that you just inherited this, but we should do it at some point in the future

Thanks @arbulu89 for the review and feedback. I completely agree on unifying the json outputs. Created an issue for that #385

PD: I see a lot of fixtures. We had some of them before already. Please, have a look if any of them got deprecated and we should remove the file

I think I didn't see the fixtures you're referring to sweat_smile . Where should I look for those?

There are some files in the test folder itself. We didn't put them as fixtures, so now we have a combinations of fixtures in the fixtures folder and other directly in the test folder.

Forget about it in this PR at least. We will do some cleanup at some point!

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

Successfully merging this pull request may close these issues.

None yet

3 participants