Skip to content
This repository was archived by the owner on Nov 27, 2024. It is now read-only.

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

Merged
melinath merged 16 commits into
GoogleCloudPlatform:mainfrom
JashanBITS:main
Mar 20, 2023
Merged

Changes to ancestrymanager logic to parse "parent" field for project, folder and organization asset types.#1450
melinath merged 16 commits into
GoogleCloudPlatform:mainfrom
JashanBITS:main

Conversation

@JashanBITS

@JashanBITS JashanBITS commented Mar 13, 2023

Copy link
Copy Markdown
Contributor

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 review from a team and shuyama1 and removed request for a team March 13, 2023 09:22
@google-cla

google-cla Bot commented Mar 13, 2023

Copy link
Copy Markdown

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
Copy Markdown
Contributor

/gcbrun

@jashan-g

Copy link
Copy Markdown
Contributor

/gcbrun

@shuyama1 shuyama1 left a comment

Copy link
Copy Markdown
Member

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.

@melinath melinath left a comment

Copy link
Copy Markdown
Member

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
Comment thread ancestrymanager/ancestryutil.go Outdated
return folderID.(string), ok
}
// folder name is store in display_name for google_folder
folderID, ok = tfData.GetOk("display_name")

Copy link
Copy Markdown
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
Copy Markdown
Contributor

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.

Comment thread ancestrymanager/ancestryutil.go
Comment thread ancestrymanager/ancestryutil.go Outdated
Comment thread ancestrymanager/ancestrymanager.go Outdated

@melinath melinath left a comment

Copy link
Copy Markdown
Member

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