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

Add utility to parse standard policy path #1218

Merged
merged 1 commit into from
Jun 10, 2024
Merged

Conversation

annakhm
Copy link
Collaborator

@annakhm annakhm commented May 28, 2024

This utility will be used in generated resources

@annakhm annakhm requested review from salv-orlando and ksamoray May 28, 2024 03:33
@@ -188,6 +188,55 @@ func getResourceIDFromResourcePath(rPath string, rType string) string {
return ""
}

func parseStandardPolicyPath(path string) ([]string, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you just need to collect the list of identifiers in the policy path, or will it also matter figuring out what is what?

For instance in:
/orgs/myorg/projects/myproj/infra/tier-1s/mygw1"
parent[2] is a router id

but in:
/orgs/myorg/projects/myproj/vpcs/myvpc/tier-1s/mygw1
parent[2] is a VPC id

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parent list will be used as parameters for API calls, so in case of orgs/projects/vpc the ids will be used as parameters in API call in the same way as gw id or segment id would be used.

@ksamoray
Copy link
Collaborator

Can we utilize this (or share code with) in nsxtPolicyPathResourceImporterHelper? I've already added VPC support in PR #1217

@annakhm annakhm force-pushed the policy-path-parser branch from b626744 to 66e3ed1 Compare May 28, 2024 22:58
@annakhm
Copy link
Collaborator Author

annakhm commented May 28, 2024

Can we utilize this (or share code with) in nsxtPolicyPathResourceImporterHelper? I've already added VPC support in PR #1217

Good point, I think nsxtPolicyPathResourceImporterHelper can use somewhat enhanced version of parseStandardPolicyPath. I can refactor it in a follow up OR when all involved PRs are merged

@annakhm annakhm force-pushed the policy-path-parser branch from 66e3ed1 to a5473d2 Compare May 31, 2024 02:01
Copy link
Member

@salv-orlando salv-orlando left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, lmk if we can do a little bit more for testing.

nsxt/policy_utils_test.go Show resolved Hide resolved

func TestParseStandardPolicyPath(t *testing.T) {

testData := []policyPathTest{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add negative test with invalid path and verify code fails as expected.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a negative test below

}
if infraPath {
// continue after infra marker
if segments[idx] != "infra" && segments[idx] != "global-infra" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember a case where we can have global-infra for a project-scoped resource.
At the same time, I don't think that a special case to handle only "infra" when in org scope is mandatory

@annakhm annakhm force-pushed the policy-path-parser branch from a5473d2 to 7a02a18 Compare June 4, 2024 21:54
In addition, add helper to get or generate ID with parent path
These helpers will be used in generated resources

Signed-off-by: Anna Khmelnitsky <[email protected]>
@annakhm annakhm force-pushed the policy-path-parser branch from 7a02a18 to fd8496b Compare June 5, 2024 17:55
Copy link
Member

@salv-orlando salv-orlando left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I have already been pedant enough here...

@annakhm annakhm merged commit d56363d into master Jun 10, 2024
3 checks passed
@annakhm annakhm deleted the policy-path-parser branch August 7, 2024 20:36
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.

3 participants