Skip to content
This repository has been archived by the owner on May 15, 2023. It is now read-only.

Changes to ancestrymanager logic to parse "parent" field for project, folder and organization asset types. #1450

Merged
merged 16 commits into from
Mar 20, 2023

Conversation

JashanBITS
Copy link
Contributor

@JashanBITS JashanBITS commented Mar 13, 2023

The PR handles ancestryManager module for the following changes:

  1. google_org_policy_policy: The parent information resides in {parent} field of the plan file. The existing logic only looks up {folder_id}, {org_id} etc. for the same. The PR also does a lookup on the {parent} field, after matching for assetType.
  2. google_folder: Parent maybe an organization or folder, so it does lookup for both.

@JashanBITS JashanBITS requested a review from a team as a code owner March 13, 2023 09:22
@JashanBITS JashanBITS requested review from shuyama1 and removed request for a team March 13, 2023 09:22
@google-cla
Copy link

google-cla bot commented Mar 13, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@jashan-g
Copy link
Collaborator

/gcbrun

@jashan-g
Copy link
Collaborator

/gcbrun

Copy link
Member

@shuyama1 shuyama1 left a comment

Choose a reason for hiding this comment

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

LGTM! Adding @melinath to give a second pass, since this is my first validator PR review.

Copy link
Member

@melinath melinath left a comment

Choose a reason for hiding this comment

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

this seems reasonable; could you add some test cases to the unit tests for each file?

@melinath melinath self-requested a review March 14, 2023 22:46
@@ -104,6 +112,17 @@ func getFolderFromResource(tfData resources.TerraformResourceData) (string, bool
if ok {
return folderID.(string), ok
}
// folder name is store in display_name for google_folder
folderID, ok = tfData.GetOk("display_name")
Copy link
Member

Choose a reason for hiding this comment

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

There are a number of "display_name" fields on different resources that generally hold "human-friendly" names for resources, so we can't rely on it to hold a folder identifier. Even for folder - I am pretty sure the display_name isn't used in the ancestry. The value returned from this function will be used to cache the ancestry of the folder, so we need to be able to rely on it to be in the standard format - which is folders/XXXXX.

Looking at the resource - you'll probably need to either:

  1. use folder.id and check with a regex that it exactly matches the expected format (because every terraform resource has an id field
  2. use folder_id but add the folders/ prefix back in - because the folder resource strips it out.

No matter which you choose, you'll need to make sure (with new test cases) that folder will return a valid ancestry even if folder_id is present on the resource with just a number in it. (Currently, this function assumes that folder_id contains something like folders/XXXX because on most resources, that's correct.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have removed the code fetching {display_name} as an ancestor for google_folder. As discussed in offline conversation, ancestry path will be determined by {parent} field when the {folder_id} for google_folder is not yet set.

ancestrymanager/ancestryutil.go Outdated Show resolved Hide resolved
ancestrymanager/ancestrymanager.go Outdated Show resolved Hide resolved
Copy link
Member

@melinath melinath left a comment

Choose a reason for hiding this comment

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

LGTM!

@melinath melinath merged commit 1f5cad8 into GoogleCloudPlatform:main Mar 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants