Conversation
This is a breaking change for the serve interface so we can easily add more options without breaking the interface and move errors to compile time instead of runtime.
cc8f95e to
8aebb93
Compare
| cmd.Flags().StringVar(&network, "network", "tcp", `the network must be "tcp", "tcp4", "tcp6", "unix" or "unixpacket"`) | ||
| cmd.Flags().Var(logLevel, "log-level", fmt.Sprintf("log level. one of: %s", strings.Join(logLevel.Allowed, ","))) | ||
| cmd.Flags().Var(logFormat, "log-format", fmt.Sprintf("log format. one of: %s", strings.Join(logFormat.Allowed, ","))) | ||
| cmd.Flags().BoolVar(&noSentry, "no-sentry", false, "disable sentry") |
There was a problem hiding this comment.
| cmd.Flags().BoolVar(&noSentry, "no-sentry", false, "disable sentry") | |
| cmd.Flags().BoolVar(&noSentry, "no-sentry", false, "disable Sentry telemetry") |
There was a problem hiding this comment.
Should we maybe use no-telemetry rather than no-sentry? We may want to change the underlying provider at some point, and developers shouldn't need to care too much about what we are using
There was a problem hiding this comment.
I think it should be two different stuff as it's error-reporting and telemetry. another option telemtry=error-reporting,all,none
There was a problem hiding this comment.
I think telemetry usually means "anything that phones home", including metrics, logs and error reports. So no-telemetry would not send any data to us at all. That's probably granular enough for now, and we can introduce further options in the future if we feel it becomes necessary?
| } | ||
| } | ||
|
|
||
| func Destination(plugin plugins.DestinationPlugin, opts ...DestinationOption) { |
There was a problem hiding this comment.
Are we happy with the inconsistency that destination plugins use an interface while source plugins use a struct pointer? Should we perhaps make both the same? Asking because now would be the best time to change it 😅
There was a problem hiding this comment.
I think yes but destinations interface we still have time to change (I remember you had that code somewhere).
There was a problem hiding this comment.
That's fine, as long as the change we plan on making is to make the destination into a struct pointer as well (and yeah, I had a branch doing that a while back, can dig it up again if we need it)
|
@erezrokah can you remind me how you ignore a linter warning - It complains rightfully about duplicate code but I think in this case it is better to have the duplicate rather then functions that abstract that given that it is two different entry points. |
This should fix it d6560bb |
🤖 I have created a release *beep* *boop* --- ## [0.7.0](v0.6.4...v0.7.0) (2022-09-19) ### ⚠ BREAKING CHANGES * Idiomatic serve interface (#126) ### Features * Add version flag ([#127](#127)) ([7e7f1ba](7e7f1ba)) * Idiomatic serve interface ([#126](#126)) ([5f848de](5f848de)) * Use JSON tag for column name when applicable ([#112](#112)) ([3aa795b](3aa795b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This is a breaking change for the serve interface so we can easily add more options without breaking the interface and move errors to compile time instead of runtime.
This should be the last breaking change before v2 release this week as I want to make sure our SDK API is extensible because we wont be breaking it for a while as SDK breaking changes are very expensive.
Summary
Use the following steps to ensure your PR is ready to be reviewed
go fmtto format your code 🖊golangci-lint run🚨 (install golangci-lint here)