Added new resource "LbEdgeExtension"#15456
Conversation
|
Hello! I am a robot. Tests will require approval from a repository maintainer to run. Googlers: For automatic test runs see go/terraform-auto-test-runs. @melinath, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
|
/gcbrun |
melinath
left a comment
There was a problem hiding this comment.
I've kicked off a test run in case there are failures. I looked through the code & it generally looks reasonable; just one change suggestion for the update tests.
...party/terraform/services/networkservices/resource_network_services_lb_edge_extension_test.go
Show resolved
Hide resolved
...party/terraform/services/networkservices/resource_network_services_lb_edge_extension_test.go
Show resolved
Hide resolved
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_network_services_lb_edge_extension" "primary" {
extension_chains {
extensions {
authority = # value needed
timeout = # value needed
}
}
}
Missing service labelsThe following new resources do not have corresponding service labels:
If you believe this detection to be incorrect please raise the concern with your reviewer. Googlers: This error is safe to ignore once you've completed go/fix-missing-service-labels. |
Non-exercised tests🔴 Tests were added that are skipped in VCR:
Tests analyticsTotal tests: 73 Click here to see the affected service packages
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
|
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
| lb_edge_extension_name: 'elb-edge-ext' | ||
| wasm_plugin_name: 'elb-wasm-plugin-data' | ||
| repository_name: 'repository-standard' | ||
| skip_test: https://github.com/GoogleCloudPlatform/magic-modules/pull/12275#discussion_r2283175770 |
There was a problem hiding this comment.
It looks like you found a way to set up a wasm_plugin in the update test - very cool! In that case, rather than generating a skipped test here, you can just exclude the example from test generation altogether.
| skip_test: https://github.com/GoogleCloudPlatform/magic-modules/pull/12275#discussion_r2283175770 | |
| exclude_test: true |
melinath
left a comment
There was a problem hiding this comment.
please also fix the missing tests.
- Removed extension fields authority and timeout;
|
Hi, I removed the two extension fields |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_network_services_lb_edge_extension" "primary" {
description = # value needed
extension_chains {
extensions {
fail_open = # value needed
forward_headers = # value needed
name = # value needed
service = # value needed
supported_events = # value needed
}
match_condition {
cel_expression = # value needed
}
name = # value needed
}
forwarding_rules = # value needed
labels = # value needed
load_balancing_scheme = # value needed
location = # value needed
name = # value needed
}
Missing service labelsThe following new resources do not have corresponding service labels:
If you believe this detection to be incorrect please raise the concern with your reviewer. Googlers: This error is safe to ignore once you've completed go/fix-missing-service-labels. |
Tests analyticsTotal tests: 72 Click here to see the affected service packages
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
|
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
melinath
left a comment
There was a problem hiding this comment.
Could you add the missing tests? These were previously in the (skipped) example so they weren't detected as "missing" (even though they were).
|
@melinath This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
melinath
left a comment
There was a problem hiding this comment.
It looks like the tests aren't getting detected due to the use of fmt.Sprint. I'm actually not 100% sure whether switching to string concatenation will fix the problem, but fmt.Sprint isn't a common pattern / both args are strings, so I'd prefer to eliminate it regardless. If the tests still aren't detected I'll just verify that all fields are present manually.
P.S. Thanks for making sure updatable fields were being updated!
| } | ||
|
|
||
| func testAccNetworkServicesLbEdgeExtension_networkServicesLbEdgeExtensionBasicCreate(context map[string]interface{}) string { | ||
| return fmt.Sprint(testAccNetworkServicesWasmPlugin_artifactRegistryRepositorySetup(context), acctest.Nprintf(` |
There was a problem hiding this comment.
| return fmt.Sprint(testAccNetworkServicesWasmPlugin_artifactRegistryRepositorySetup(context), acctest.Nprintf(` | |
| return testAccNetworkServicesWasmPlugin_artifactRegistryRepositorySetup(context) + acctest.Nprintf(` |
| } | ||
|
|
||
| func testAccNetworkServicesLbEdgeExtension_networkServicesLbEdgeExtensionBasicUpdate(context map[string]interface{}) string { | ||
| return fmt.Sprint(testAccNetworkServicesWasmPlugin_artifactRegistryRepositorySetup(context), acctest.Nprintf(` |
There was a problem hiding this comment.
| return fmt.Sprint(testAccNetworkServicesWasmPlugin_artifactRegistryRepositorySetup(context), acctest.Nprintf(` | |
| return testAccNetworkServicesWasmPlugin_artifactRegistryRepositorySetup(context) + acctest.Nprintf(` |
| } | ||
| } | ||
| } | ||
| `, context)) |
There was a problem hiding this comment.
| `, context)) | |
| `, context) |
| } | ||
| } | ||
| } | ||
| `, context)) |
There was a problem hiding this comment.
| `, context)) | |
| `, context) |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 76 Click here to see the affected service packages
Action takenFound 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
|
sweet, looks like that fixed the test detection |
melinath
left a comment
There was a problem hiding this comment.
Overall this looks good. I have a few nits but I'll just apply those & queue this for merge if everything's good.
| description: | | ||
| A Common Expression Language (CEL) expression that is used to match requests for which the extension chain is executed. | ||
| required: true | ||
| - name: 'extensions' |
There was a problem hiding this comment.
It looks like not all the subfields here are supported; that's not a blocker for the initial implementation but wanted to flag it for potential follow-up: https://docs.cloud.google.com/service-extensions/docs/reference/rest/v1/ExtensionChain#extension Possibly they're not valid for this type of object though!
| base_url: '{{op_id}}' | ||
| result: | ||
| resource_inside_response: false | ||
| custom_code: |
There was a problem hiding this comment.
nit
| custom_code: |
| guides: | ||
| 'Configure a edge extension': 'https://cloud.google.com/service-extensions/docs/configure-edge-extensions' | ||
| api: 'https://cloud.google.com/service-extensions/docs/reference/rest/v1beta1/projects.locations.lbEdgeExtensions' | ||
| docs: |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 78 Click here to see the affected service packages
Action takenFound 10 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
melinath
left a comment
There was a problem hiding this comment.
LGTM - failing tests are unrelated
ce790e6
Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
Adds new resource
google_network_services_lb_edge_extension.Fixes: #24173
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.