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

Onboard IBM Telemetry #1874

Open
wants to merge 17 commits into
base: theiliad-v10-legacy
Choose a base branch
from

Conversation

makafsal
Copy link
Member

@makafsal makafsal commented Jul 29, 2024

Updates

  • Core:
    • added packages/core/telemetry.yml file
    • installed dependency @ibm/telemetry-js
    • included collection notice packages/core/README.md

@makafsal makafsal marked this pull request as ready for review July 30, 2024 09:00
@makafsal makafsal requested review from theiliad, Akshat55 and a team as code owners July 30, 2024 09:00
Copy link
Member

@mattrosno mattrosno left a comment

Choose a reason for hiding this comment

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

Project IDs and configs look good! I think we just need to uninstall the legacy @carbon/telemetry packages now that they are no longer used in postinstall scripts.

packages/angular/package.json Show resolved Hide resolved
packages/core/package.json Outdated Show resolved Hide resolved
packages/react/package.json Outdated Show resolved Hide resolved
packages/svelte/package.json Outdated Show resolved Hide resolved
packages/vue/package.json Outdated Show resolved Hide resolved
@mattrosno
Copy link
Member

It's interesting that @carbon/telemetry was installed in the packages, but not called in postinstall scripts. I didn't check git history, but maybe they were instrumented with the legacy telemetry at some point but then disabled, but not uninstalled.

@makafsal makafsal requested a review from mattrosno August 5, 2024 05:49
Copy link
Member

@mattrosno mattrosno left a comment

Choose a reason for hiding this comment

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

@IgnacioBecerra @ariellalgilmore this LGTM. I reviewed:

  • Project IDs match the IDs used in the master branch
  • YML config files have the right scopes and the React config file has the same JSX config as the master branch
  • Postinstall scripts
  • Readme notices
  • The package.json's don't specify "files", so there's no need to include the package.json in those export lists

@mattrosno
Copy link
Member

@theiliad I'm not sure if that Mend check can be ignored or not, but the changes in this PR LGTM to instrument the v10 packages.

@makafsal
Copy link
Member Author

makafsal commented Aug 21, 2024

@mattrosno If there’s anything I need to do on my end, please let me know - I’m happy to help.

@theiliad
Copy link
Member

@mattrosno @makafsal there isn't an automatic process to make 0.x releases, I will do it manually soon

@mattrosno
Copy link
Member

@theiliad following up as this it still open. Thanks!

Copy link
Contributor

@IgnacioBecerra IgnacioBecerra left a comment

Choose a reason for hiding this comment

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

@makafsal Since we are still in the process of onboarding the v10 charts package with this PR, I attempted to push my own commits here, but due to lacking permissions, I am suggesting changes through a review instead.

We are looking to update the READMEs to the wording of the latest version, and also updating the .yml files so we're using the new stable URL. Let me know if you have any question, thank you!

packages/angular/README.md Outdated Show resolved Hide resolved
packages/core/README.md Outdated Show resolved Hide resolved
packages/react/README.md Outdated Show resolved Hide resolved
packages/svelte/README.md Outdated Show resolved Hide resolved
packages/vue/README.md Outdated Show resolved Hide resolved
packages/vue/telemetry.yml Outdated Show resolved Hide resolved
packages/svelte/telemetry.yml Outdated Show resolved Hide resolved
packages/react/telemetry.yml Outdated Show resolved Hide resolved
packages/core/telemetry.yml Outdated Show resolved Hide resolved
packages/angular/telemetry.yml Outdated Show resolved Hide resolved
makafsal and others added 10 commits October 17, 2024 22:01
Co-authored-by: Ignacio Becerra <i1becerr@ucsd.edu>
Co-authored-by: Ignacio Becerra <i1becerr@ucsd.edu>
Co-authored-by: Ignacio Becerra <i1becerr@ucsd.edu>
Co-authored-by: Ignacio Becerra <i1becerr@ucsd.edu>
Co-authored-by: Ignacio Becerra <i1becerr@ucsd.edu>
Co-authored-by: Ignacio Becerra <i1becerr@ucsd.edu>
Co-authored-by: Ignacio Becerra <i1becerr@ucsd.edu>
Co-authored-by: Ignacio Becerra <i1becerr@ucsd.edu>
Co-authored-by: Ignacio Becerra <i1becerr@ucsd.edu>
Co-authored-by: Ignacio Becerra <i1becerr@ucsd.edu>
@makafsal
Copy link
Member Author

@IgnacioBecerra Great, I've committed those changes 🚀. If you have more changes to commit, don't hesitate to ping me in Slack✌️.

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.

4 participants