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: Remove plugin option for logging error events to Sentry #1724

Merged
merged 3 commits into from
Jun 11, 2024

Conversation

murarustefaan
Copy link
Contributor

@murarustefaan murarustefaan commented Jun 6, 2024

Closes #1809.

@@ -306,12 +268,7 @@ func (s *PluginServe) newCmdPluginServe() *cobra.Command {
cmd.Flags().StringVar(&otelEndpointURLPath, "otel-endpoint-urlpath", "", "Open Telemetry HTTP collector endpoint URL path")
cmd.Flags().StringArrayVar(&otelEndpointHeaders, "otel-endpoint-headers", []string{}, "Open Telemetry HTTP collector endpoint headers")
cmd.Flags().BoolVar(&otelEndpointInsecure, "otel-endpoint-insecure", false, "use Open Telemetry HTTP endpoint (for development only)")
cmd.Flags().BoolVar(&noSentry, "no-sentry", false, "disable sentry")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure here tbh what would be the best approach, whether to remove this or have it do nothing and mark as deprecated. Wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

I would keep the option but make it deprecated and no-op.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, I wasn't sure if deleting it was the best decision as if it was like that, I'd have had to delete it from the other plugin sdks as well.

Copy link
Member

@erezrokah erezrokah Jun 11, 2024

Choose a reason for hiding this comment

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

Yeah I think if we remove the flag new plugins might not work with older CLI versions (don't remember if we're doing strict parsing)

Copy link

github-actions bot commented Jun 6, 2024

⏱️ Benchmark results

Comparing with 7b9d6cc

  • Glob-8 ns/op: 90.73 ⬇️ 0.76% decrease vs. 7b9d6cc

@murarustefaan murarustefaan requested a review from disq June 10, 2024 13:09
@github-actions github-actions bot added feat and removed feat labels Jun 10, 2024
@murarustefaan murarustefaan merged commit 7732fe8 into main Jun 11, 2024
10 checks passed
@murarustefaan murarustefaan deleted the feat/1809-remove-sentry-logging branch June 11, 2024 14:29
kodiakhq bot pushed a commit that referenced this pull request Jun 14, 2024
🤖 I have created a release *beep* *boop*
---


## [4.45.0](v4.44.2...v4.45.0) (2024-06-14)


### Features

* Remove plugin option for logging error events to Sentry ([#1724](#1724)) ([7732fe8](7732fe8))


### Bug Fixes

* **deps:** Update module github.com/cloudquery/plugin-pb-go to v1.19.19 ([#1726](#1726)) ([a1dd044](a1dd044))
* Don't include other relation siblings if not specified in config ([#1720](#1720)) ([f730ec5](f730ec5))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants