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

Expose extended attribute in realization info data source #1089

Merged
merged 2 commits into from
Feb 15, 2024

Conversation

GraysonWu
Copy link
Collaborator

Fixes #793

@GraysonWu GraysonWu requested review from ksamoray and annakhm January 26, 2024 19:10
@ksamoray
Copy link
Collaborator

Should we be able to retrieve multiple attributes for the same resource?
Also, why is this a part of the generic realization info, instead of being part of the tier1 datasource?

@annakhm
Copy link
Collaborator

annakhm commented Jan 29, 2024

Should we be able to retrieve multiple attributes for the same resource? Also, why is this a part of the generic realization info, instead of being part of the tier1 datasource?

If we choose to expose extended attributes for all objects, I would suggest to return the whole set as a Map type, so that the user would be able to use it as follows: data.nsxt_policy_realization_info.info.extended_attributes["ip_address"]

@GraysonWu
Copy link
Collaborator Author

Should we be able to retrieve multiple attributes for the same resource? Also, why is this a part of the generic realization info, instead of being part of the tier1 datasource?

If we choose to expose extended attributes for all objects, I would suggest to return the whole set as a Map type, so that the user would be able to use it as follows: data.nsxt_policy_realization_info.info.extended_attributes["ip_address"]

Yes, using a map could be a better option. But there might be multiple entities for a specific entity_type. For example, a user wants to know the IP addresses. Multiple RealizedLogicalRouterPort entities will be returned, and only one of it have IP addresses. We are not sure which attribute users care to decide which entity we want to return.

Our current logic is: if entity_type is not specified, we return the first one. If entity_type is specified, we return the first one matching the entity_type. So I added the extended_attribute_key to let the users specify which key they want. If multiple entities have the key, the first one will be returned.

@GraysonWu GraysonWu force-pushed the realization-info-add-ip branch from 2ad949d to 0a5dac5 Compare January 31, 2024 00:50
@GraysonWu
Copy link
Collaborator Author

Should we be able to retrieve multiple attributes for the same resource? Also, why is this a part of the generic realization info, instead of being part of the tier1 datasource?

Because in API, those IP addresses are not exposed in tier1, only in realization info. But we could call realization info API behind the scenes for tier1 data source, which may also require a polling mechanism to wait for that logical router port to be in the REALIZED state. Glad to hear your ideas thx.

@ksamoray
Copy link
Collaborator

Should we be able to retrieve multiple attributes for the same resource? Also, why is this a part of the generic realization info, instead of being part of the tier1 datasource?

Because in API, those IP addresses are not exposed in tier1, only in realization info. But we could call realization info API behind the scenes for tier1 data source, which may also require a polling mechanism to wait for that logical router port to be in the REALIZED state. Glad to hear your ideas thx.

Or require the user to add a dependency between the T1 data source and the realization resource if the IPs are needed - we can document this if we don't trust the user's intuition 😁

Either way these are just proposals - I'm good with any solution.

@GraysonWu GraysonWu force-pushed the realization-info-add-ip branch 3 times, most recently from 0d028ac to b97fa11 Compare February 8, 2024 21:19
@annakhm
Copy link
Collaborator

annakhm commented Feb 13, 2024

LGTM, but lets add a test for this data source? We could piggyback on existing test for gateway interface resource and add this data source.

@vmwclabot
Copy link
Member

@GraysonWu, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

@vmwclabot vmwclabot added the dco-required DCO Required label Feb 14, 2024
@GraysonWu
Copy link
Collaborator Author

/test-all

@GraysonWu GraysonWu force-pushed the realization-info-add-ip branch from 77a339d to a5157da Compare February 14, 2024 02:33
@vmwclabot vmwclabot removed the dco-required DCO Required label Feb 14, 2024
@GraysonWu
Copy link
Collaborator Author

/test-all

@GraysonWu GraysonWu force-pushed the realization-info-add-ip branch from a5157da to aa34f79 Compare February 14, 2024 03:23
@GraysonWu
Copy link
Collaborator Author

/test-all

@GraysonWu GraysonWu merged commit 4f31cce into vmware:master Feb 15, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Attribute uplink_interface_ips for Ressource/Data-Source nsxt_policy_tier1_gateway
4 participants