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(analytics): include project and user metadata when running commands with noProject #4915

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

mkhq
Copy link
Contributor

@mkhq mkhq commented Aug 2, 2023

What this PR does / why we need it:

Optionally includes project and user metadata when running a command with noProject = true. Internally, we instantiate a DummyGarden instance which is not having project or user metadata available. In addition, we only set the enterpriseDomain field if its defined in the project config.

To achieve this there are a couple of notable changes:

  • The global store is extended with a cloud user profile including userId and organizationName for each associated domain
  • A profile request is made when logging in to a cloud domain to ensure the existence of the user and to store the metadata in the global store upon login
  • When analytics is called with a DummyGarden-instance, it tries to fetch the project name and cloud domain from the config + does a lookup of the cloud user profile
  • Mocha tests are called with an additional flag to handle nock and fetch compatibility (Node 18+: make nock work native fetch nock/nock#2397)
  • Version checks are disabled in tests unless explicitly part of the tests. This was always the case for circle CI which is why the nocked version check service tests always failed.
  • Extends the pendingRequests for identify analytics events to make sending them more robust
  • Only sets the cloudDomain when logged in
  • Only sets the enterpriseDomain and enterpriseProjectId when defined in the project config

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

  • Auth token validity check is best effort, opted for that approach now instead of pulling in a JWT library

@mkhq mkhq force-pushed the analytics-load-project-config branch from 4a992e6 to e0a906f Compare August 3, 2023 11:38
@mkhq mkhq force-pushed the analytics-load-project-config branch 3 times, most recently from 2781a47 to eea61e2 Compare August 30, 2023 12:43
@mkhq mkhq requested a review from a team August 30, 2023 12:43
@mkhq mkhq marked this pull request as ready for review August 30, 2023 13:35
@mkhq mkhq force-pushed the analytics-load-project-config branch from eea61e2 to 632004c Compare September 6, 2023 13:50
core/src/cloud/api.ts Outdated Show resolved Hide resolved
@mkhq mkhq force-pushed the analytics-load-project-config branch from 632004c to 2bef65f Compare September 17, 2023 10:20
@mkhq mkhq force-pushed the analytics-load-project-config branch from 321e17f to c2ea32d Compare September 26, 2023 08:11
@mkhq mkhq requested a review from eysi09 October 2, 2023 07:31
Copy link
Collaborator

@eysi09 eysi09 left a comment

Choose a reason for hiding this comment

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

Great stuff!

I had a couple of comments but otherwise looks good.

this.identify({
userId: this.cloudUserId,
anonymousId: anonymousUserId,
traits: {
userIdV2,
customer: cloudUser?.organization.name,
customer: cloudUser?.organizationName,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is kind of an awkward name. Should we skip this for non-enterprise?

await CloudApi.saveAuthToken({ log, globalConfigStore, tokenResponse, domain: cloudDomain })

try {
cloudApi = await CloudApi.factory({ log, cloudDomain, skipLogging: true, globalConfigStore })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we already do this in the in line 92?


let projectConfig: ProjectConfig | undefined

if (garden instanceof DummyGarden) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we extract this to a separate function and update the control flow so that it doesn't have all these nested if statements?

We can instead return early with something like:

if (!projectConfig) {
  return
}

// proceed...

@mkhq mkhq force-pushed the analytics-load-project-config branch from c2ea32d to 0cad985 Compare October 17, 2023 08:22
@mkhq mkhq force-pushed the analytics-load-project-config branch 2 times, most recently from f12ffca to 0f564f9 Compare October 25, 2023 07:19
@mkhq mkhq force-pushed the analytics-load-project-config branch from 0f564f9 to 1d826c5 Compare October 25, 2023 12:28
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.

None yet

3 participants