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

fix: not ingesting tenant count during azure analysis #328

Merged
merged 3 commits into from
Jan 29, 2024
Merged

Conversation

mistahj67
Copy link
Contributor

Description

When selecting “All Azure Tenants” in Data Quality, the statistics table shows 0 (zero) tenants.

Motivation and Context

All Azure Tenants stats should accurately display tenant count.

How Has This Been Tested?

Integration test has been added.

Screenshots (if appropriate):

Types of changes

  • Chore (a change that does not modify the application functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • Documentation updates are needed, and have been made accordingly.
  • I have added and/or updated tests to cover my changes.
  • All new and existing tests passed.
  • My changes include a database migration.

cmd/api/src/analysis/azure/queries_test.go Outdated Show resolved Hide resolved
KeyVaults: 1,
Relationships: 188,
}
_, agg, err := azure2.GraphStats(context.TODO(), testCtx.GraphDB)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted to skip verifying the individual stats slice of structs here because building the expected result involves retrieving a runId which is generated inside of graphStats fn itself, as well as all the hardcode above x12. If anyone changes the harnesses, the agg will fail this test anyway. That said, I'm open to any solutions that might get us a less brittle test here.

@@ -48,7 +48,7 @@ func GraphStats(ctx context.Context, db graph.Database) (model.AzureDataQualityS
runID = newUUID.String()
}

return stats, aggregation, db.ReadTransaction(ctx, func(tx graph.Transaction) error {
err := db.ReadTransaction(ctx, func(tx graph.Transaction) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is just for readability.

@mistahj67 mistahj67 force-pushed the BED-3858 branch 4 times, most recently from b3159ac to b6789d2 Compare January 24, 2024 17:33
@@ -1042,6 +1068,7 @@ type AZInboundControlHarness struct {

func (s *AZInboundControlHarness) Setup(testCtx *GraphTestContext) {
tenantID := RandomObjectID(testCtx.testCtx)
s.Tenant = testCtx.NewAzureTenant(tenantID)
Copy link
Contributor

Choose a reason for hiding this comment

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

The JSON/SVGs will need to be updated here like we did for the other one above

Copy link
Contributor

@irshadaj irshadaj left a comment

Choose a reason for hiding this comment

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

Looks good to me apart from the changes we discussed (added in comments). Go ahead and merge once those updates are made!

@mistahj67 mistahj67 merged commit f7502b1 into main Jan 29, 2024
3 checks passed
@mistahj67 mistahj67 deleted the BED-3858 branch January 29, 2024 20:54
@github-actions github-actions bot locked and limited conversation to collaborators Jan 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants