-
Notifications
You must be signed in to change notification settings - Fork 25
[VC-43753] CyberArk(client): add CyberArk snapshot conversion #684
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: master
Are you sure you want to change the base?
Conversation
9606752
to
4d8e691
Compare
88c1cc7
to
8b9a233
Compare
2d44e46
to
34d67da
Compare
caadaf0
to
e8f50da
Compare
78700c9
to
b7adba8
Compare
a331e26
to
0bfb107
Compare
d82113d
to
8842333
Compare
e8f50da
to
7686607
Compare
8842333
to
5a2ae3c
Compare
ee3d84d
to
a04cdad
Compare
- Introduced `convertDataReadings` to process `DataReading` objects into snapshots. - Added support for extracting Kubernetes server version and dynamic resources. - Updated `CyberArkClient` to use the new data conversion logic. - Refactored `DiscoveryData` and `DynamicData` structures for better type safety. - Replaced `unstructured.Unstructured` with `runtime.Object` in `Snapshot` fields. - Enhanced `DataGathererDiscovery` and `DataGathererDynamic` to return strongly typed data. - Added unit tests for new data extraction and conversion functions. Signed-off-by: Richard Wall <[email protected]>
a04cdad
to
00e4e91
Compare
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.
Pull Request Overview
This PR adds CyberArk snapshot conversion functionality to process DataReading objects into the snapshot format expected by the CyberArk API.
- Introduced data conversion logic with
convertDataReadings
and extractor functions for processing different resource types - Updated type definitions to use
runtime.Object
instead ofunstructured.Unstructured
for better type safety - Enhanced data gatherers to return strongly typed data structures (
DynamicData
andDiscoveryData
)
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
pkg/client/client_cyberark.go | Added conversion logic and extractor functions for transforming DataReadings to CyberArk snapshots |
pkg/client/client_cyberark_convertdatareadings_test.go | Added comprehensive test coverage for data conversion functions |
pkg/client/client_cyberark_test.go | Added helper function for generating test data and updated test cases |
pkg/internal/cyberark/dataupload/dataupload.go | Changed Snapshot field types from unstructured.Unstructured to runtime.Object |
pkg/internal/cyberark/dataupload/dataupload_test.go | Updated test to use version constant instead of hardcoded value |
pkg/internal/cyberark/dataupload/mock.go | Added assertions for cluster ID and agent version validation |
pkg/datagatherer/k8s/discovery.go | Refactored to return strongly typed DiscoveryData instead of generic map |
pkg/datagatherer/k8s/dynamic.go | Refactored to return strongly typed DynamicData instead of generic map |
pkg/datagatherer/k8s/dynamic_test.go | Updated tests to work with new strongly typed return values |
api/datareading.go | Added DynamicData and DiscoveryData type definitions |
examples/machinehub.yaml | Updated configuration to include comprehensive resource gathering definitions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
func convertDataReadings[T any]( | ||
extractorFunctions map[string]func(*api.DataReading, *T) error, | ||
readings []*api.DataReading, | ||
target *T, |
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.
[nitpick] The generic type parameter T any
is too permissive. Consider constraining it to *dataupload.Snapshot
or create an interface that defines the expected behavior, as this function appears to be specifically designed for snapshot conversion.
func convertDataReadings[T any]( | |
extractorFunctions map[string]func(*api.DataReading, *T) error, | |
readings []*api.DataReading, | |
target *T, | |
func convertDataReadings( | |
extractorFunctions map[string]func(*api.DataReading, *dataupload.Snapshot) error, | |
readings []*api.DataReading, | |
target *dataupload.Snapshot, |
Copilot uses AI. Check for mistakes.
// Temporary hard coded cluster ID. | ||
// TODO(wallrj): The clusterID will eventually be extracted from the supplied readings. | ||
snapshot.ClusterID = "success-cluster-id" |
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.
The hardcoded cluster ID 'success-cluster-id' should be replaced with a more appropriate default or configuration-based value for production use. Consider using a UUID or extracting it from cluster metadata.
Copilot uses AI. Check for mistakes.
Add CyberArk snapshot conversion
convertDataReadings
to processDataReading
objects into snapshots.CyberArkClient
to use the new data conversion logic.DiscoveryData
andDynamicData
structures for better type safety.unstructured.Unstructured
withruntime.Object
inSnapshot
fields.DataGathererDiscovery
andDataGathererDynamic
to return strongly typed data.Part of: https://venafi.atlassian.net/browse/VC-43753
Followup PRs
Testing