MMv1: Add list-resource Generation#17514
Conversation
|
Hi there, I'm the Modular magician. I've detected the following information about your changes for commit 4533c2a: Diff reportYour PR generated the following diffs in downstream repositories:
Test reportAnalytics
Affected Service Packages
Step 1: Replaying Mode Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
View the build log Step 2: Recording Mode
Caution Issues requiring attention before PR completion 🔴 Initial Recording Failed: Some tests failed during the recording step. See the table above for details. Please address these issues to complete your PR. If you believe these detections are incorrect or unrelated to your change, please raise the concern with your reviewer. View the build log or the debug logs folder for detailed results. @BBBmau, @c2thorn, @NickElliot VCR tests complete for 4533c2a! |
NickElliot
left a comment
There was a problem hiding this comment.
could you revise the unit test issue causing this PR to fail the "unused-template-check"?
@NickElliot the unused-template-check comes from the template files being introduced not being found in use within product/ directories. this makes sense that it's failing since the list template files are used within terraform.go the same as resources. These unused-template-checks would be ignored once we have this PR merged as they'll be treated the same way as resources in magic-modules. correct me if im wrong @c2thorn |
@BBBmau I don't think the tool correctly recognizes template files that aren't meant to be referenced directly in YAML files. I think files like this would need to be ignored in https://github.com/GoogleCloudPlatform/magic-modules/blob/main/tools/template-check/cmd/unusedtmpl.go#L123. Can either @shuyama1 or @iyabchen confirm? I'm assuming if we just merge it won't fail downstream, but I'm not sure. |
| if len(markers) > 0 { | ||
| keys = append(keys, markers[len(markers)-1][1]) | ||
| } | ||
| return keys |
There was a problem hiding this comment.
Should we be length checking keys here? if for some reason it's zero, should it just go ahead and error out instead of propagating through?
There was a problem hiding this comment.
I don't think a length check is necessary since we do that within the Capture function. in generated resources the capture method takes the value returned from `ListResultDisplayNameKeyStrings1
magic-modules/mmv1/third_party/terraform/acctest/test_utils.go.tmpl
Lines 102 to 116 in f42abb2
|
|
||
| func (r Resource) ListResultDisplayNameKeyStrings() []string { | ||
| var keys []string | ||
| if slices.ContainsFunc(r.RootProperties(), func(p *Type) bool { return p.Name == "display_name" }) { |
There was a problem hiding this comment.
maybe being able to override this in the YAML could be a useful feature for some resources. this is probably the best default
There was a problem hiding this comment.
I didn't run into a resource where we needed to explicitly set the display_name for list representation. We can add in a list_display_name_override field in the event that we do need this for a specific resource. I'd lean towards adding this once a resource does come up that requires this.
| }.Register() | ||
| } | ||
|
|
||
| var _ list.ListResource = &{{ $.ResourceName -}}ListResource{} |
There was a problem hiding this comment.
Unrelated to your PR, but why is the official name a list RESOURCE
In my mind resources vs data sources were separated by the CUD parts of CRUD
isn't this more of a list data source? :)
There was a problem hiding this comment.
good question, it's been tricky navigating naming since i do agree we aren't technically creating a resource but more calling existing resources (which exactly matches what data source is)
List Source probably would've been better? Reviewed the list resources docs and mentions the following
List resources are an abstraction that allows Terraform to search for a specific resource type within a given scope, for example listing all EC2 instances within an AWS account or all virtual networks within a resource group. Taking a list configuration as input, remote objects are retrieved and returned to Terraform as list result data which is displayed to the user.
| @@ -0,0 +1,123 @@ | |||
| // Copyright IBM Corp. 2014, 2026 | |||
| @@ -0,0 +1,99 @@ | |||
| {{ define "listResourceMethod" }} | |||
There was a problem hiding this comment.
This template looks consistent with the handwritten resource
There was a problem hiding this comment.
FYI i'll be pushing one more change to this once this PR gets merged:
Co-authored-by: Cameron Thornton <camthornton@google.com>
|
Hi there, I'm the Modular magician. I've detected the following information about your changes for commit 63b0154: Diff reportYour PR generated the following diffs in downstream repositories:
Test reportAnalytics
Affected Service Packages
Step 1: Replaying Mode Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
View the build log Step 2: Recording Mode
Caution Issues requiring attention before PR completion 🔴 Initial Recording Failed: Some tests failed during the recording step. See the table above for details. Please address these issues to complete your PR. If you believe these detections are incorrect or unrelated to your change, please raise the concern with your reviewer. View the build log or the debug logs folder for detailed results. |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes for commit 711b9b7: Diff reportYour PR generated the following diffs in downstream repositories:
Test reportAnalytics
Affected Service Packages
Step 1: Replaying Mode Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
View the build log Step 2: Recording Mode
Caution Issues requiring attention before PR completion 🔴 Initial Recording Failed: Some tests failed during the recording step. See the table above for details. Please address these issues to complete your PR. If you believe these detections are incorrect or unrelated to your change, please raise the concern with your reviewer. View the build log or the debug logs folder for detailed results. |
Thanks for flagging! Yeah, this type of template files should probably be excluded from this check. This is non blocking and I'll look into how to fix it. The check only targets new changes, so merging this won't break other PRs. Sorry for the noise. |
|
Created #17594 to fix the unused-template-check |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes for commit 4bb46c3: Diff reportYour PR generated the following diffs in downstream repositories:
Test reportAnalytics
Affected Service Packages
Step 1: Replaying Mode Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
View the replaying VCR build log Step 2: Recording Mode
Caution Issues requiring attention before PR completion 🔴 Initial Recording Failed: Some tests failed during the recording step. See the table above for details. Please address these issues to complete your PR. If you believe these detections are incorrect or unrelated to your change, please raise the concern with your reviewer. View the recording VCR build log or the debug logs folder for detailed results. @BBBmau, @c2thorn, @NickElliot VCR tests complete for 4bb46c3! |
|
Thanks @shuyama1 ! PR ready for review once more after rebase and updates to |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes for commit b1b4e5d: Diff reportYour PR generated the following diffs in downstream repositories:
Test reportAnalytics
Affected Service Packages
Step 1: Replaying Mode Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
View the replaying VCR build log Step 2: Recording Mode
Caution Issues requiring attention before PR completion 🔴 Initial Recording Failed: Some tests failed during the recording step. See the table above for details. Please address these issues to complete your PR. If you believe these detections are incorrect or unrelated to your change, please raise the concern with your reviewer. View the recording VCR build log or the debug logs folder for detailed results. @BBBmau, @c2thorn, @NickElliot VCR tests complete for b1b4e5d! |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes for commit f146f4c: Diff reportYour PR generated the following diffs in downstream repositories:
Test reportAnalytics
Affected Service Packages
Step 1: Replaying Mode Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
View the replaying VCR build log Step 2: Recording Mode
Caution Issues requiring attention before PR completion 🔴 Initial Recording Failed: Some tests failed during the recording step. See the table above for details. Please address these issues to complete your PR. If you believe these detections are incorrect or unrelated to your change, please raise the concern with your reviewer. View the recording VCR build log or the debug logs folder for detailed results. @BBBmau, @c2thorn, @NickElliot VCR tests complete for f146f4c! |
This PR introduces support for list resource generation in magic-modules.
It allows the ability to generate by simply opting a resource in with
generate_list_resource: trueIt's worth noting that in the current state it does not cover cases where test_vars need to be updated as well as the case where HCL attribute pathes are provided in test configurations. This will be addressed in a follow-up PR that focues on these two cases. Draft can be found here:
MMv1: addListScopeBIndingsoftest_var&&HCL attribute pathfields forquery_testgeneration BBBmau/magic-modules#30The new generated files will be the following:
below is a passing test with
cloud_run_service, the first list resource that will be introduced undermagic-modules:Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.