-
Notifications
You must be signed in to change notification settings - Fork 876
fix: locate Terraform entrypoint file #16753
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
Conversation
if (filename === defaultMainTerraformFile) { | ||
initialFile = path; | ||
skip = true; | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the order in which traverse()
will traverse the nodes of fileTree
? Are child paths sorted prior to being added to the frontier? In other words, if you have both a/main.tf
and z/main.tf
, which will get chosen? I know you have a test above, but is the output dependant on the ordering of the nodes? Is it just "first found wins"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me expand traverse()
with sorting capabilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice 👍
@@ -90,7 +90,7 @@ export const TemplateVersionEditorPage: FC = () => { | |||
// File navigation | |||
// It can be undefined when a selected file is deleted | |||
const activePath: string | undefined = | |||
searchParams.get("path") ?? findInitialFile(fileTree ?? {}); | |||
searchParams.get("path") ?? findEntrypointFile(fileTree ?? {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know what happens if searchParams.get
returns an empty string or a string that isn't a valid filename?
That would short-circuit the nullish expression and prevent findEntrypointFile
from ever being called, and as far as I can tell, activePath
gets fed directly into the editor without any additional data massaging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch! let me look into this 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 1. If "main.tf" exists in root, return it. | ||
// 2. Traverse through sub-directories. | ||
// 3. If "main.tf" exists in a sub-directory, skip further browsing, and return the path. | ||
// 4. If "main.tf" was not found, return the last reviewed "".tf" file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the sorting in place now, I feel like it would've helped to clarify what "last reviewed" means, especially to help make it clear that the tests are actually deterministic
Fixes: #16360
This PR updates the template version editor code to accurately identify the Terraform entrypoint file.