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

chore: stop v1 analytics - cli-569 #5568

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sandor-trombitas
Copy link
Contributor

@sandor-trombitas sandor-trombitas commented Nov 7, 2024

Pull Request Submission Checklist

  • Follows CONTRIBUTING guidelines
  • Includes detailed description of changes
  • Contains risk assessment (Low | Medium | High)
  • Highlights breaking API changes (if applicable)
  • Links to automated tests covering new functionality
  • Includes manual testing instructions (if necessary)
  • Updates relevant GitBook documentation (PR link: ___)

What does this PR do?

Stop sending v1 analytics

Where should the reviewer start?

Review code changes

How should this be manually tested?

Run a command in debug mode and look for analytics disabled

Copy link
Contributor

github-actions bot commented Nov 7, 2024

Warnings
⚠️

Since the CLI is unifying on a standard and improved tooling, we're starting to migrate old-style imports and exports to ES6 ones.
A file you've modified is using either module.exports or require(). If you can, please update them to ES6 import syntax and export syntax.
Files found:

  • src/lib/analytics/index.ts
⚠️ There are multiple commits on your branch, please squash them locally before merging!

Generated by 🚫 dangerJS against 067c782

@sandor-trombitas sandor-trombitas changed the title CLI-569-stop-v1-analytics CLI-569: stop v1 analytics Nov 7, 2024
@sandor-trombitas sandor-trombitas changed the title CLI-569: stop v1 analytics cli-569: stop v1 analytics Nov 7, 2024
@sandor-trombitas sandor-trombitas force-pushed the CLI-569-stop-v1-analytics branch 2 times, most recently from 97fb41e to 4ed271b Compare November 7, 2024 10:08
@sandor-trombitas sandor-trombitas changed the title cli-569: stop v1 analytics chore: stop v1 analytics - cli-569 Nov 7, 2024
@sandor-trombitas sandor-trombitas marked this pull request as ready for review November 7, 2024 10:17
@sandor-trombitas sandor-trombitas requested a review from a team as a code owner November 7, 2024 10:17
@sandor-trombitas sandor-trombitas force-pushed the CLI-569-stop-v1-analytics branch 3 times, most recently from 5206019 to da38e11 Compare November 7, 2024 11:38
} else {
return true;
}
// Analytics disabled until we can migrate to v2 analytics
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: I'm not sure this comment makes sense :) maybe just remove it

);
expect(stdout).toContain(project.path('package.json'));

const requests = server.getRequests().filter((req: any) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion: for this test, it might be worth to just remove the part that uses the analytics data to assert and leave the rest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants