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 plan-time validation of name on google_compute_instance #11886

Merged

Conversation

karolgorc
Copy link
Contributor

@karolgorc karolgorc commented Oct 2, 2024

closes hashicorp/terraform-provider-google#18265

  • Added name validation to google_compute_instance

The regexes are wrong in the validation file. Our verification functions expected 2-63 characters when in reality it's 1-63. Basing on this documentation: https://cloud.google.com/compute/docs/naming-resources#resource-name-format and manual testing

Some other tests might fail due to these changes in the validate file so pls paste the test errors after the CICD run

Release Note Template for Downstream PRs (will be copied)

compute: added plan-time validation to `name` on `google_compute_instance`

@github-actions github-actions bot requested a review from rileykarson October 2, 2024 07:31
Copy link

github-actions bot commented Oct 2, 2024

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

@rileykarson, 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 Oct 2, 2024
Copy link

github-actions bot commented Oct 7, 2024

@rileykarson 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

github-actions bot commented Oct 9, 2024

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

fix unit test expecting hardcoded values
fix ordering scratch disks in the state
Remove hardcoded values that are handled by the API
@@ -22,7 +22,7 @@ const (

SubnetworkLinkRegex = "projects/(" + ProjectRegex + ")/regions/(" + RegionRegex + ")/subnetworks/(" + SubnetworkRegex + ")$"

RFC1035NameTemplate = "[a-z](?:[-a-z0-9]{%d,%d}[a-z0-9])"
RFC1035NameTemplate = "[a-z]([-a-z0-9]{%d,%d}[a-z0-9])?"
Copy link
Member

Choose a reason for hiding this comment

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

This allows the string "a" when any input is supplied. For example, 1,61 (source input to the fn of 2,63). Added some extra test cases as constraints.

I suspect we need to rework the regex substantially to go below a minimum of 2 while properly validating the format. We may want to mix procedural code and regex or just write this as entirely procedural code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed. After checking this regex fails on one new test case that you've added. This is mainly due to length which should be procedural imo.

The only other think that this regex is affecting are Service Account names so will look out for errors there as well.

@github-actions github-actions bot requested a review from rileykarson October 11, 2024 07:28
@karolgorc karolgorc force-pushed the instance-name-validation branch from 34f96b1 to 823b3e7 Compare October 11, 2024 08:52
@karolgorc
Copy link
Contributor Author

sorry. Pushed a rebased branch on accident.

What was done.

  • added new test cases
  • The function now validates the length outside of the regex.
  • The variable with the regex takes in one string value instead of 2 ints
RFC1035NameTemplate = "[a-z]([-a-z0-9]%v[a-z0-9])?"

This allows to input a "*" wildcard for our case but also can take "{4,28}" as input not to break the existing ServiceAccount regexes. Because they specify length in the regex while we do it through the function.

Here are 2 unit tests affected

=== RUN   TestValidateRFC1035Name
--- PASS: TestValidateRFC1035Name (0.00s)

=== RUN   TestValidateServiceAccountLink
--- PASS: TestValidateServiceAccountLink (0.00s)

Copy link

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

@modular-magician modular-magician added service/compute-instances and removed awaiting-approval Pull requests that need reviewer's approval to run presubmit tests labels Oct 17, 2024
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google-beta provider: Diff ( 3 files changed, 34 insertions(+), 19 deletions(-))

Errors

google provider:

  • Failed to build branch auto-pr-11886-old

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 4196
Passed tests: 3776
Skipped tests: 418
Affected tests: 2

Click here to see the affected service packages

All service packages are affected

Action taken

Found 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
  • TestAccBigqueryConnectionConnection_bigqueryConnectionKmsExample
  • TestAccDataLossPreventionDiscoveryConfig_Update

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

🟢 Tests passed during RECORDING mode:
TestAccBigqueryConnectionConnection_bigqueryConnectionKmsExample[Debug log]
TestAccDataLossPreventionDiscoveryConfig_Update[Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.


🟢 All tests passed!

View the build log or the debug log for each test

@karolgorc
Copy link
Contributor Author

==> Checking that code complies with gofmt requirements...
gofmt needs running on the following files:
./google/services/sql/resource_sql_database_test.go
You can use the command: `make fmt` to reformat code.
make: *** [GNUmakefile:26: fmtcheck] Error 1

doesn't seem related

Copy link

@GoogleCloudPlatform/terraform-team @rileykarson 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

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

amanMahendroo pushed a commit to amanMahendroo/magic-modules that referenced this pull request Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing plan-time validation for the name attribute of the google_compute_instance
4 participants