-
Notifications
You must be signed in to change notification settings - Fork 451
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
feat: sort the variables in the same order from input variables.tf file. #2591
feat: sort the variables in the same order from input variables.tf file. #2591
Conversation
@qz267 LGTM! Can we add a test scenario for this! |
if hclDiags.HasErrors() { | ||
Log.Info("Failed to parse HCL: ", "diags: ", hclDiags) | ||
} | ||
variableContent, _, hclDiags := variableFile.Body.PartialContent(variableSchema) |
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.
Would there be problem here if there was an error in variables.tf file parsing?
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.
How about calling log.Error()
instead? Or do you have any suggestions?
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.
My recommendation would be to propagate the error from this function (change signature to (map[string]int, error) and only sort if no error, otherwise log and continue so we degrade gracefully. IRL if this throws an error, we would not reach here as it will likely be caught earlier in tfconfig.LoadModule.
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.
Thanks for the PR! Some minor suggestions
if hclDiags.HasErrors() { | ||
Log.Info("Failed to parse HCL: ", "diags: ", hclDiags) | ||
} | ||
variableContent, _, hclDiags := variableFile.Body.PartialContent(variableSchema) |
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.
My recommendation would be to propagate the error from this function (change signature to (map[string]int, error) and only sort if no error, otherwise log and continue so we degrade gracefully. IRL if this throws an error, we would not reach here as it will likely be caught earlier in tfconfig.LoadModule.
cli/bpmetadata/tfconfig.go
Outdated
variableOrderKeys[block.Labels[0]] = i | ||
|
||
} | ||
Log.Info("Found variables in order: ", "variableOrderKeys: ", variableOrderKeys) |
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's remove this, I had it just for debugging in the internal poc
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
configPath string | ||
expectOrders map[string]int | ||
}{ | ||
{ |
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.
Could we add a few more cases with no outputs and invalid output block declaration?
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
…y names when failed to get the original customer sort order
#2591 (comment) |
Use hclparse to read order of variables from variables.tf. Sort the metadata.yaml with variables in this obtained order.