feat: Update SQL Server auditing filter to be more flexible #8314
Conversation
…s using the value filter
dslagle
force-pushed
the
sql-server-auditing
branch
from
08c0b84
to
f151a38
Compare
|
@ajkerrigan @thisisshi This one is ready for review when you are able, thanks! |
| @@ -335,52 +335,69 @@ class AuditingFilter(Filter): | |||
|
|
|||
| """ | |||
|
|
|||
| cache_key = 'c7n:auditing-settings' | |||
There was a problem hiding this comment.
It doesn't look like we have a consistent way to name this type of variable for the azure provider. In AWS it's most commonly annotation_key, but in Azure I see a pretty even mix of cache_key, annotation_key and <attribute>_key, and <attribute>_property. It's probably worth converging on (and documenting) an Azure convention going forward.
cache_key seems clear enough in this context, though I can also see that causing confusion with the cache_key used with the local disk cache.
There was a problem hiding this comment.
I know this is closed now, but the idea of annotations is something that I don't fully understand. My assumption has been that it is meant as an addition to the resource data that would show up in the final output for a matched resource. However, I'm not aware of any documentation around their use, and the confusing part is that the annotation also tends to end up being a local cache for filter data which is separate from the more generic cache you mention.
All that to say I agree there should be a standard for handling what is a pretty common pattern across filters.
| if self.is_legacy: | ||
| return auditing_enabled == self.enabled | ||
| # otherwise process the auditing settings using ValueFilter logic for full flexibility | ||
| else: | ||
| return super().__call__(resource['properties'][self.cache_key]) |
There was a problem hiding this comment.
I like this clear/explicit handling of the previous behavior 👍
No description provided.