-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Making network field optional in Backupdr MS #11575
Conversation
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. |
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_backup_dr_management_server" "primary" {
networks {
network = # value needed
}
}
|
Tests analyticsTotal tests: 1 Click here to see the affected service packages
View the build log |
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.
Apologies if there's a limitation I've missed, but could you add a test (or probably better to modify an existing test) to not include these fields? This will help verify that they are in fact not required.
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_backup_dr_management_server" "primary" {
networks {
network = # value needed
}
}
|
Tests analyticsTotal tests: 3 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
|
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_backup_dr_management_server" "primary" {
networks {
network = # value needed
}
}
|
Tests analyticsTotal tests: 3 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
|
|
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_backup_dr_management_server" "primary" {
networks {
network = # value needed
}
}
|
Tests analyticsTotal tests: 3 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
|
|
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_backup_dr_management_server" "primary" {
networks {
network = # value needed
}
}
|
Tests analyticsTotal tests: 3 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
|
|
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.
It looks like there is still a test failure, potentially for another test. But to be clear, I don't think every existing test should be changed (unless you are doing that for another reason, like performance?). I expected there to be one test with this field removed, to prove it is not required.
@niharika-98, this PR is waiting for action from you. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. If no action is taken, this PR will be closed in 28 days. This notification can be disabled with the |
Tomorrow, the Magic Modules repository is scheduled to undergo a language migration from Ruby to Go. You can view more details about this in our announcement here: hashicorp/terraform-provider-google#19583 (or go/mm-migration-announcement if you are a Googler) This open pull request may become incompatible due to most YAML and .erb files converting to Go-compatible files. However, the changes seem minor enough that editing the converted files should not be too difficult. The main difficulty will be rebasing after the migration, but I suggest simply opening a new pull request. That should be easier overall. Let us know if you have any questions or issues. |
@niharika-98, this PR is waiting for action from you. If no action is taken, this PR will be closed in 14 days. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. This notification can be disabled with the |
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: 55 Click here to see the affected service packages
🟢 All tests passed! View the build log |
Release Note Template for Downstream PRs (will be copied)