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

Reintroduce telemetry #1004

Merged
merged 3 commits into from
Sep 7, 2023
Merged

Reintroduce telemetry #1004

merged 3 commits into from
Sep 7, 2023

Conversation

JulesFaucherre
Copy link
Contributor

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have checked for similar issues and haven't found anything relevant.
  • This is not a security issue (which should be reported here: https://circleci.com/security/)
  • I have read Contribution Guidelines.

Changes

With cobra, when you define a PersistentPreRun in a child command, you overwrite all the command's parents PersistentPreRun cobra issue. When introducing telemetry, we used the PersistentPreRun to send the events, this made the runner commands not call the PersistentPreRun in cmd/runner/runner.go which defines the opts.r value. In the children commands the opts.r was nil, thus causing nil pointer deference panic.

This PR makes the runner command use the PreRunE to send their telemetry events to avoid overwriting the parent's PersistentPreRun.

Rationale

This PR is here in reaction to this CLI telemetry retintroduction. This feature made the runner test crash because it was introducing a nil pointer dereference panic like explained in the Changes part. The feature was then reverted. This PR aims at reintroducing the feature with the necessary fix.

Note to the reviewer: it is greatly advised not to look at the first commit as it is a revert commit. For more information about this commit content do look into the original PR

Considerations

About the decision of the fix, two possibilities were considered: wrapping the PreRunE that was already used by all the commands to make it send the telemetry events or send the telemetry event directly in the Run of every command. The direction of the PreRunE was selected because it was easier to implement, more generic and thus would be less likely to be forgotten in the case of the addition of other commands

With cobra, when you define a PersistentPreRun in a child command, you
overwrite all the command's parents PersistentPreRun. When introducing
telemetry, we used the PersistentPreRun to send the events, this made
the runner commands not call the PersistentPreRun in
cmd/runner/runner.go which defines the 'opts.r' value. In the commands
the 'opts.r' was nil, thus causing nil pointer deference panic.

This commit uses the PreRunE to send the telemetry event instead of the
PersistentPreRun to avoid this issue.
@JulesFaucherre JulesFaucherre merged commit b3874b3 into develop Sep 7, 2023
@JulesFaucherre JulesFaucherre deleted the reintroduce-telemetry branch September 7, 2023 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants