Skip to content

Usage Collection#1203

Merged
Brian Strauch (brianstrauch) merged 26 commits intomainfrom
usage
Jun 30, 2022
Merged

Usage Collection#1203
Brian Strauch (brianstrauch) merged 26 commits intomainfrom
usage

Conversation

@brianstrauch
Copy link

@brianstrauch Brian Strauch (brianstrauch) commented Mar 4, 2022

Checklist

  1. [CRUCIAL] Is the change for CP or CCloud functionalities that are already live in prod?
    • yes: ok

What

Added a wrapper around the CLI to collect usage data, with a special case for --help which does not call the PostRun function. (I'm using the PostRun here since the PreRun is overly complex and needs a major refactor).

Added a new make target, update-whitelist, which makes a PR to the cc-cli-service with the whitelist for the next version.
Example PR: https://github.com/confluentinc/cc-cli-service/pull/40

@brianstrauch Brian Strauch (brianstrauch) marked this pull request as ready for review May 4, 2022 23:00
@brianstrauch Brian Strauch (brianstrauch) requested a review from a team as a code owner May 4, 2022 23:00
Copy link
Contributor

@DABH David Hyde (DABH) left a comment

Choose a reason for hiding this comment

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

LGTM once tests are there; only one trivial question

github.com/confluentinc/ccloud-sdk-go-v2/iam v0.6.0
github.com/confluentinc/ccloud-sdk-go-v2/org v0.5.0
github.com/confluentinc/ccloud-sdk-go-v2/service-quota v0.1.0
github.com/confluentinc/cire-bucket-service/protos/bucket v0.50.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this public or private? Would like to not introduce new private dependencies

Choose a reason for hiding this comment

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

Just investigated, and this was incorrectly marked as "indirect" earlier. I must've forgot to go mod tidy because it was in the wrong section.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok well regardless... is there any (relatively easy) way to eliminate this dependency? 😅 If we need to save that for another PR, no worries...

Choose a reason for hiding this comment

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

Oh wait, this needed to be added for the BYOK Kafka integration test. So looks like we need it.

Copy link
Contributor

@MuweiHe MuweiHe left a comment

Choose a reason for hiding this comment

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

Nice work!!

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