feat: support destination based configuration#169
Conversation
betinacosta
changed the title
cassiofariasmachado
left a comment
cassiofariasmachado
left a comment
There was a problem hiding this comment.
Nice work so far! I've added some comments
|
|
||
| client = create_client( | ||
| destination_name="my-audit-destination", | ||
| destination_instance="my-binding-instance", |
There was a problem hiding this comment.
destination_instance could be optional also and we can use default by default
There was a problem hiding this comment.
I believe on agw we don't have destination name and destination_instance configuration, since this is padronized on runtime.
There was a problem hiding this comment.
Do you think we should remove these parameters and consider them by default?
There was a problem hiding this comment.
Right now, the only "mandatory" parameter is destination_name if endpoint, namespace and development_id are not passed. Do you guys think it works this way?
| ) | ||
|
|
||
| dest_client = _dest_create_client(instance=destination_instance) | ||
| options = ConsumptionOptions(fragment_name=fragment_name) if fragment_name else None |
There was a problem hiding this comment.
We must also inform the fragment_level=ConsumptionLevel.SUBACCOUNT in the ConsumptionOptions.
Ignore this comment if the default level is subaccount for this parameter.
There was a problem hiding this comment.
Fragment Level is not on instance level? It should be specific to tenant if I remember correctly, and then we would need to ask this information too
There was a problem hiding this comment.
I tested passing the fragment with subaccount level and I got the necessary information back without errors. But I don't know if this will be the case for always. I will change to have subaccount as default.
There was a problem hiding this comment.
It's tenant specific, but it was created in the subaccount level not in the instance level during the SPII. Either the destination and the fragment are being created in the subaccount level but tenant specific.
There was a problem hiding this comment.
I added the level to the fragment options, do you think any other information is required?
| ready-to-use AuditClient. | ||
|
|
||
| Usage: | ||
| Usage — explicit config:: |
There was a problem hiding this comment.
could we just add it as Usage: and then add comments, I believe this is how we usually do, but I can be worng
| endpoint = destination.url | ||
| props = destination.properties | ||
|
|
||
| deployment_id = props.get(_DESTINATION_PROP_DEPLOYMENT_ID) or "" |
There was a problem hiding this comment.
Maybe better to have an enum for this dest properties?
There was a problem hiding this comment.
I think is better as well. Fixed.
| [project] | ||
| name = "sap-cloud-sdk" | ||
| version = "0.26.1" | ||
| version = "0.26.2" |
There was a problem hiding this comment.
I don't think this is a minor since it's implementing new functionality
| version = "0.26.2" | |
| version = "0.27.0" |
There was a problem hiding this comment.
It makes sense. Updated
3 participants
Description
The AuditLog NG module currently requires callers to explicitly provide endpoint, deployment_id, and namespace when calling create_client. In SPII-based deployments, these values are stored in a Destination (with tenant-specific fragments) following the ALS SPII v3.0.0 provisioning lifecycle. This change allows create_client to resolve these parameters automatically from a Destination, reducing boilerplate and avoiding misconfiguration.
Type of Change
Please check the relevant option:
How to Test
Checklist
Before submitting your PR, please review and check the following:
Breaking Changes
If this PR introduces breaking changes, please describe:
Additional Notes
Add any additional context, screenshots, or information that would help reviewers.