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(sumologicextension): use hostname as default collector name #918

Merged
merged 1 commit into from
Feb 1, 2023

Conversation

swiatekm
Copy link

This ends up causing more problems than it solves. In most circumstances the hostname is unique, and where it isn't, the user should set the collector name explicitly. And just using the hostname makes the user experience in the majority of cases better.

@github-actions github-actions bot added the go label Jan 31, 2023
@swiatekm swiatekm force-pushed the feat/sumologicextension/collector-guid branch from 2d69b2b to fefd3d1 Compare January 31, 2023 14:24
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jan 31, 2023
@swiatekm swiatekm force-pushed the feat/sumologicextension/collector-guid branch from fefd3d1 to 25c57d1 Compare January 31, 2023 14:24
@swiatekm swiatekm marked this pull request as ready for review January 31, 2023 14:25
@swiatekm swiatekm requested a review from a team as a code owner January 31, 2023 14:25
Copy link
Contributor

@sumo-drosiek sumo-drosiek left a comment

Choose a reason for hiding this comment

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

Should we clobber by default then?

@swiatekm
Copy link
Author

Should we clobber by default then?

I think the default behaviour is better in this case. Clobber would actually delete the existing collector, which is a destructive action I'd rather not do without explicit user opt-in. Maybe we should log a warning in this case?

@swiatekm swiatekm force-pushed the feat/sumologicextension/collector-guid branch from 25c57d1 to ec50e64 Compare January 31, 2023 16:28
@swiatekm
Copy link
Author

Should we clobber by default then?

I think the default behaviour is better in this case. Clobber would actually delete the existing collector, which is a destructive action I'd rather not do without explicit user opt-in. Maybe we should log a warning in this case?

I added it.

@swiatekm swiatekm requested a review from sumo-drosiek January 31, 2023 16:34
Comment on lines +437 to +440
if collectorName != resp.CollectorName {
se.logger.Warn("Collector name already in use, registered modified name", zap.String("registered_name", resp.CollectorName))
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, but not sure if we should mention clobber or any documentation here

Copy link
Author

Choose a reason for hiding this comment

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

Hm, I probably should add this behaviour to the documentation to begin with. I don't want to make this log message too long.

Copy link
Author

Choose a reason for hiding this comment

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

Updated documentation.

@swiatekm swiatekm force-pushed the feat/sumologicextension/collector-guid branch from ec50e64 to 43fed5d Compare February 1, 2023 10:38
@swiatekm swiatekm requested a review from sumo-drosiek February 1, 2023 11:07
@swiatekm swiatekm merged commit 20446ac into main Feb 1, 2023
@swiatekm swiatekm deleted the feat/sumologicextension/collector-guid branch February 1, 2023 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation go
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants