Skip to content
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

Add edgenetwork Router resource. #12594

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

f1urps
Copy link
Contributor

@f1urps f1urps commented Dec 17, 2024

Create Router resource in product edgenetwork.

Fixes hashicorp/terraform-provider-google#15666

Tests are disabled for this resource for the same reason as other resources in edgenetwork. See: #8905

This PR depends on another in-review PR: #12586

Release Note Template for Downstream PRs (will be copied)

`google_edgenetwork_router`

@github-actions github-actions bot requested a review from roaks3 December 17, 2024 16:29
Copy link

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

@roaks3, 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.

@modular-magician modular-magician added the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Dec 17, 2024
Copy link

@roaks3 This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

Copy link

@GoogleCloudPlatform/terraform-team @roaks3 This PR has been waiting for review for 1 week. Please take a look! Use the label disable-review-reminders to disable these notifications.

Copy link
Contributor

@roaks3 roaks3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly needs more testing (unless the resource is immutable), but otherwise LGTM

mmv1/products/edgenetwork/Router.yaml Outdated Show resolved Hide resolved
@@ -0,0 +1,82 @@

resource "google_edgenetwork_router" "{{$.PrimaryResourceId}}" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the Router resource is not marked as immutable, and a number of resources are therefore presumed to be mutable in-place. If that's the case, this resource will need an update test to verify that those fields can be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make an exception and skip this requirement? Due to the dependence on physical hardware, the edgenetwork API can only be tested manually by the internal dev team anyway, so I don't think those sorts of unit tests are useful in this case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to write the test but have it be skipped, so that your team can run it as a one off prior to submission?

I understand the constraints will be difficult to work around, but conversely, this resource implementation could be broken and we would have no way of knowing until customers use it. If you have some other way to validate the correct mutability properties for each of these fields perhaps we could rely on that, but otherwise I think we want to at least see output from a passing test before submitting.

@github-actions github-actions bot requested a review from roaks3 December 31, 2024 20:24
Copy link

github-actions bot commented Jan 3, 2025

@roaks3 This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

type: KeyValueLabels
description: |
Labels associated with this resource.
required: false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'm still seeing a lot of required: false which is creating some noise when reading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-approval Pull requests that need reviewer's approval to run presubmit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Router resource for edgenetwork API
3 participants