runtime: Correct naming & docs for version checking#8191
runtime: Correct naming & docs for version checking#8191charlieegan3 merged 2 commits intoopen-policy-agent:mainfrom
Conversation
| run: make ci-build-windows ci-go-ci-build-linux ci-go-ci-build-linux-static | ||
| timeout-minutes: 30 | ||
| env: | ||
| TELEMETRY_URL: ${{ secrets.TELEMETRY_URL }} |
There was a problem hiding this comment.
I understand these are no longer needed since we have a GitHub.com default now.
| tlsCertRefresh time.Duration | ||
| ignore []string | ||
| serverMode bool | ||
| skipVersionCheck bool // skipVersionCheck is deprecated. Use disableTelemetry instead |
There was a problem hiding this comment.
This field used to be deprecated before the telemetry feature was originally implemented, back in ~0.20, but I think it makes sense to go back to using this flag now since it's still here and is well named.
| // For backwards compatibility, check if `--skip-version-check` flag set. | ||
| if params.skipVersionCheck { | ||
| // For backwards compatibility, check if deprecated `--disable-telemetry` flag set. | ||
| if params.disableTelemetry { |
There was a problem hiding this comment.
I think we need to support this path for some time as this same commit deprecates the flag.
| | DOCKER_USER | Docker username for uploading release images. Will be used with `docker login`. Optional -- If not provided the image push steps are skipped. | | ||
| | DOCKER_PASSWORD | Docker password or API token for the configured `DOCKER_USER`. Will be used with `docker login`. Optional -- If not provided the image push steps are skipped. | | ||
| | SLACK_NOTIFICATION_WEBHOOK | Slack webhook for sending notifications. Optional -- If not provided the notification steps are skipped. | | ||
| | TELEMETRY_URL | URL to inject at build-time for OPA version reporting. Optional -- If not provided the default value in OPA's source is used. | |
There was a problem hiding this comment.
This is still set in GH, but I not used and should be removed after merge.
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| User-Agent: "Open Policy Agent/v0.12.3 (darwin, amd64)" | ||
| GET /repos/open-policy-agent/opa/releases/latest HTTP/1.1 | ||
| Host: api.github.com | ||
| User-Agent: OPA-Version-Checker |
There was a problem hiding this comment.
I have updated the code to use a custom user agent with no version info.
|
|
||
| // WithTelemetryGatherers allows registration of telemetry gatherers which enable injection of additional data in the | ||
| // telemetry report | ||
| func WithTelemetryGatherers(gs map[string]report.Gatherer) func(*Manager) { |
There was a problem hiding this comment.
I removed this since it's based on an internal type. I was unsure what to do otherwise.
There was a problem hiding this comment.
I thought I remembered EOPA using this, but we have ripped it all out since then. IIRC, you could provide a map[string]func(context.Context) (any, error) and it would work, regardless of the report.Gatherer type.
There was a problem hiding this comment.
Ahh yeah that makes much more sense. I have made some updates in d19852e
ca9b573 to
ef6110a
Compare
| } | ||
| m.reporter = reporter | ||
|
|
||
| m.reporter.RegisterGatherer("min_compatible_version", func(_ context.Context) (any, error) { |
There was a problem hiding this comment.
This was used for telemetry but is no longer used as far as I can see.
9d303a1 to
510def7
Compare
srenatus
left a comment
There was a problem hiding this comment.
Nice cleanup, thanks a lot. And it makes so much more sense to call it "versioncheck" these days. 👍
|
|
||
| // WithTelemetryGatherers allows registration of telemetry gatherers which enable injection of additional data in the | ||
| // telemetry report | ||
| func WithTelemetryGatherers(gs map[string]report.Gatherer) func(*Manager) { |
There was a problem hiding this comment.
I thought I remembered EOPA using this, but we have ripped it all out since then. IIRC, you could provide a map[string]func(context.Context) (any, error) and it would work, regardless of the report.Gatherer type.
510def7 to
326d4ac
Compare
There was a problem hiding this comment.
Huh, what was that about? 🤔 Ah, the change in CLI parameters. Nevermind.
326d4ac to
f7e6e1a
Compare
Rename telemetry functionality to version checking to accurately reflect current behavior following open-policy-agent#7756. The system only checks GitHub releases for version updates without sending any data about the OPA instance and so the privacy docs have been updated too. Signed-off-by: Charlie Egan <charlie_egan@apple.com>
Deprecate WithTelemetryGatherers since telemetry gathering has been removed. The function now returns a no-op to maintain API compatibility without breaking existing code that might uses it. Signed-off-by: Charlie Egan <charlie_egan@apple.com>
d19852e to
a0ba825
Compare
|
Thanks for the review Stephan! |
…t#8191) * runtime: Correct naming of version checking code Rename telemetry functionality to version checking to accurately reflect current behavior following open-policy-agent#7756. The system only checks GitHub releases for version updates without sending any data about the OPA instance and so the privacy docs have been updated too. Signed-off-by: Charlie Egan <charlie_egan@apple.com> * Make WithTelemetryGatherers a no-op Deprecate WithTelemetryGatherers since telemetry gathering has been removed. The function now returns a no-op to maintain API compatibility without breaking existing code that might uses it. Signed-off-by: Charlie Egan <charlie_egan@apple.com> --------- Signed-off-by: Charlie Egan <charlie_egan@apple.com>
Rename telemetry functionality to version checking to accurately reflect current behavior following
#7756.
The system only checks GitHub releases for version updates without sending any data about the OPA instance and so the privacy docs have been updated too.
Fixes #8093