-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
connectorEnforcement field added along with modified test #6667
connectorEnforcement field added along with modified test #6667
Conversation
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @ScottSuarez, a repository maintainer, has been assigned to assist you and help review your changes. ❓ First time contributing? Click here for more detailsYour assigned reviewer will help review your code by:
You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails. If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox. |
Could you try pulling in the latest changes from main? We recently made a CI change which may be causing failures |
…o connector-enforcement-issue-244763538
mmv1/third_party/terraform/resources/resource_sql_database_instance.go.erb
Show resolved
Hide resolved
I notice this pr is in draft. Let me know when it's ready for review |
…o connector-enforcement-issue-244763538
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccSqlDatabaseInstance_mysqlMajorVersionUpgrade|TestAccSqlDatabaseInstance_Timezone|TestAccSqlDatabaseInstance_SqlServerAuditConfig|TestAccSqlDatabaseInstance_PointInTimeRecoveryEnabled|TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange|TestAccSqlDatabaseInstance_basic_with_user_labels|TestAccSqlDatabaseInstance_authNets|TestAccSqlDatabaseInstance_settings_upgrade|TestAccSqlDatabaseInstance_settings_secondary|TestAccSqlDatabaseInstance_settings_deletionProtection|TestAccSqlDatabaseInstance_maintenanceVersion|TestAccSqlDatabaseInstance_basicMSSQL|TestAccComputeForwardingRule_update |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccSqlDatabaseInstance_settings_deletionProtection|TestAccSqlDatabaseInstance_mysqlMajorVersionUpgrade|TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange|TestAccSqlDatabaseInstance_basic_with_user_labels|TestAccSqlDatabaseInstance_SqlServerAuditConfig|TestAccSqlDatabaseInstance_authNets|TestAccSqlDatabaseInstance_settings_upgrade|TestAccSqlDatabaseInstance_PointInTimeRecoveryEnabled|TestAccSqlDatabaseInstance_basicMSSQL |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
@ScottSuarez There are 2 tests failing for the resource but it doesn't seem like that is happening due to the field I've added. Those tests are failing in the current version as well. How do I proceed now? |
/gcbrun |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccSqlDatabaseInstance_SqlServerAuditConfig|TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange|TestAccBigtableAppProfile_bigtableAppProfileAnyclusterExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Hi Jay, would you be able to file a bug with the team that owns the api failing in the test. It's not clear to me why it's failing and claims to be transient. According to our reruns. This claim is not holding up |
Actually I'm not authorised to do so, hence if possible can you do that on my behalf. |
…o connector-enforcement-issue-244763538
…o connector-enforcement-issue-244763538
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. Terraform GA: Diff ( 3 files changed, 12 insertions(+)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange|TestAccSqlDatabaseInstance_SqlServerAuditConfig |
Tests failed during RECORDING mode: Please fix these to complete your PR |
"transient error" should now be resolved /gcbrun |
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. Terraform GA: Diff ( 3 files changed, 12 insertions(+)) |
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. Terraform GA: Diff ( 3 files changed, 14 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccComputeForwardingRule_update|TestAccSqlDatabaseInstance_settings_secondary|TestAccSqlDatabaseInstance_SqlServerAuditConfig|TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Lets
These tests are failing in teamcity (our nightly runs). Could you also create a bug to track that we disabled them? |
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. Terraform GA: Diff ( 3 files changed, 18 insertions(+), 4 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease |
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. Terraform GA: Diff ( 3 files changed, 16 insertions(+)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease |
@ScottSuarez I've added skipVcr and now the checks are passing. About the failing tests, I guess Stephen has created a bug ticket for the failing 2 tests along with some other tests. |
Also, there are several test cases with Failure due to the same issue, they were already added to SkipVcr. |
Have you been able to run these tests locally and do they pass for you. I am curious if we can get a minimum config test passing. Without some sort of knowledge as to, yes I can confirm this works. I worry about merging. I could also perhaps try to run some tests locally. |
I guess it has a similar issue for all such test cases, regarding the |
Hi @ScottSuarez, Can you have a look. What I found was all the cases with Service Networking present are skipped as of now. Looks like it is blocking several cases. I guess it will be fine to proceed. Also wanted to ask, did you get any findings running it locally? |
Yeah, if we can't get any sort test to pass with this field, locally or otherwise. I'm against merging. If you can produce a configuration and deployment that works I am more comfortable checking this change in. |
Just to be clear, the |
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.
Ack, apologies here. It's been so long trying to sort this out the context was lost. Thanks for your patience here.
connector_enforcement field added to sql_database_instance resource
settings test case modified with new field
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)