-
Notifications
You must be signed in to change notification settings - Fork 530
tech-insights: ability to add filters on the checks #6697
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
base: main
Are you sure you want to change the base?
Conversation
Missing ChangesetsThe following package(s) are changed by this PR but do not have a changeset:
See CONTRIBUTING.md for more information about how to add changesets. Changed Packages
|
cf031e1 to
d16ac69
Compare
| deps: { | ||
| config: coreServices.rootConfig, | ||
| logger: coreServices.logger, | ||
| discovery: coreServices.discovery, | ||
| techInsights: techInsightsFactCheckerFactoryExtensionPoint, | ||
| }, | ||
| async init({ config, logger, techInsights }) { | ||
| async init({ config, logger, discovery, techInsights }) { | ||
| const catalogClient = new CatalogClient({ discoveryApi: discovery }); | ||
| const factory = JsonRulesEngineFactCheckerFactory.fromConfig(config, { | ||
| logger, | ||
| catalogApi: catalogClient, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better route is having the module define catalog as a dependency with catalogServiceRef from @backstage/plugin-catalog-node. This is the newer way of communicating with the catalog as it includes auth support. In this scenario, I think passing { credentials: await this.auth.getOwnServiceCredentials() } as the second argument to the catalog api calls would suffice.
deps: {
config: coreServices.rootConfig,
logger: coreServices.logger,
- discovery: coreServices.discovery,
+ catalog: catalogServiceRef,
+ auth: coreServices.auth,
techInsights: techInsightsFactCheckerFactoryExtensionPoint,
}wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it's discouraged: https://backstage.io/docs/features/software-catalog/faq/#can-i-call-the-catalog-itself-from-inside-a-processor--provider 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is referring to the ingestion and processing of entities in the catalog, known as EntityProvider and EntityProcessor. This factory class is neither a catalog provider or processor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah okay, now it make sense. sorry I misunderstood earlier. I now found the similar references:
community-plugins/workspaces/lighthouse/plugins/lighthouse-backend/src/service/EntitiesLoader.ts
Line 32 in a872eb7
const { token } = await auth.getPluginRequestToken({ catalogClient: catalogServiceRef,
I will update my code accordingly, thank you for the review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kurtaking, once again, thank you for the review, I have updated as per your suggestion.
Signed-off-by: surajnarwade <[email protected]> Signed-off-by: surajnarwade <[email protected]>
Signed-off-by: surajnarwade <[email protected]> Signed-off-by: surajnarwade <[email protected]>
Signed-off-by: surajnarwade <[email protected]>
Signed-off-by: surajnarwade <[email protected]>
Signed-off-by: surajnarwade <[email protected]>
Signed-off-by: surajnarwade <[email protected]>
Signed-off-by: surajnarwade <[email protected]>
Signed-off-by: surajnarwade <[email protected]>
Signed-off-by: surajnarwade <[email protected]>
Signed-off-by: surajnarwade <[email protected]>
Signed-off-by: Suraj Narwade <[email protected]>
e9054bb to
d4ab081
Compare
Signed-off-by: Suraj Narwade <[email protected]>
Signed-off-by: Suraj Narwade <[email protected]>
Signed-off-by: Suraj Narwade <[email protected]>
| private compareValues( | ||
| entityValue: any, | ||
| filterValue: string | symbol, | ||
| ): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feature looks great for scalar properties like kind, spec.type, spec.lifecycle, etc.
Our main use case is to drive check applicability off catalog tags, e.g.:
apiVersion: backstage.io/v1alpha1
kind: Component
metadata:
name: cloud-ui-common
tags:
- cloudui
- angular
- common
Ideally we’d configure a check like:
filter:
metadata.tags:
- cloudui
- backend
If I’m reading the current compareValues logic correctly, get(entity, 'metadata.tags') returns a string[], and the comparison ends up being ['cloudui', 'angular', 'common'] === 'cloudui', so the filter never matches.
Would you be open to extending compareValues to treat array entity values as “contains any of these filter values”? For example:
// If entityValue is an array, treat it as "contains" semantics <--- Suggested addition
if (Array.isArray(entityValue)) {
return entityValue.some(ev => this.compareValues(ev, filterValue));
}
// Handle string comparison case-insensitively for kind, type, lifecycle, etc.
// This provides a better user experience as these values are typically case-insensitive
if (typeof entityValue === 'string' && typeof filterValue === 'string') {
return entityValue.toLowerCase() === filterValue.toLowerCase();
}
// Handle symbol comparison (less common but supported for completeness)
if (typeof filterValue === 'symbol') {
return entityValue === filterValue;
}
// Default to strict equality for all other types (numbers, booleans, etc.)
return entityValue === filterValue;
That would allow metadata.tags to scope checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @PatrickFrueh, thank you so much for the review. I will test out the code for tags as well and make any necessary changes 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PatrickFrueh I have updated the PR with suggested changes and tested locally and it works 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for the quick turnaround @surajnarwade. Really appreciate you testing this locally and incorporating the change. 🚀
Signed-off-by: surajnarwade <[email protected]>
Signed-off-by: surajnarwade <[email protected]>
Signed-off-by: surajnarwade <[email protected]>
Signed-off-by: surajnarwade <[email protected]>
reference: firebase/firebase-functions#1664 Signed-off-by: surajnarwade <[email protected]>
Signed-off-by: surajnarwade <[email protected]>
Signed-off-by: surajnarwade <[email protected]>
57a994d to
2a73106
Compare
PatrickFrueh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solved from the tag-filtering use-case perspective. The changes in src/service/JsonRulesEngineFactChecker.ts:L181-184 correctly handle array values via contains semantics. LGTM 👍
Thanks again for the quick update!
This PR solves this issue: #4259
Hey, I just made a Pull Request!
✔️ Checklist
Signed-off-byline in the message. (more info)