Skip to content

Conversation

@stefbomdcs
Copy link
Contributor

@stefbomdcs stefbomdcs commented May 11, 2020

Description

Making change in SQL MI create related tests to have subnet delegated to SQL MI service as this will be forced by SQL MI and all create requests for non-delegated subnets will fail.

Checklist

  • I have read the Submitting Changes section of CONTRIBUTING.md
  • The title of the PR is clear and informative
  • The appropriate ChangeLog.md file(s) has been updated:
    • For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
  • The PR does not introduce breaking changes
  • If applicable, the changes made in the PR have proper test coverage
  • For public API changes to cmdlets:
    • a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
    • the markdown help files have been regenerated using the commands listed here

@adxsdkps
Copy link
Collaborator

Can one of the admins verify this patch?

@msftclas
Copy link

msftclas commented May 11, 2020

CLA assistant check
All CLA requirements met.

@stefbomdcs stefbomdcs changed the title Making change in tests to delegate subnet to SQL MI service Making change in SQL MI tests to delegate subnet to SQL MI service May 11, 2020
@VeryEarly
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

@VeryEarly VeryEarly requested a review from petrajkogit May 12, 2020 01:43
@VeryEarly
Copy link
Collaborator

Hi @petrajkogit ,

since the change is on Common.ps1, can you please verify whether it has impact on other test cases

@VeryEarly VeryEarly self-assigned this May 12, 2020
@stefbomdcs
Copy link
Contributor Author

stefbomdcs commented May 12, 2020

Hi @petrajkogit ,

since the change is on Common.ps1, can you please verify whether it has impact on other test cases

I've run one of the tests which is using the changed method. However, it looks like I'll need to run all tests which are using this method in recording mode, as there is one more ARM call added with this change.

Adding recording from one of the tests
@stefbomdcs
Copy link
Contributor Author

@VeryEarly
Do you maybe know why it takes this long for these checks to complete?

@VeryEarly
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

@VeryEarly
Copy link
Collaborator

@VeryEarly
Do you maybe know why it takes this long for these checks to complete?

It looks like the CI was not triggered

@stefbomdcs
Copy link
Contributor Author

@VeryEarly
Thanks for starting this. Am I missing some permission as it looks like that Merging is blocked?

@VeryEarly
Copy link
Collaborator

@stefbomdcs @petrajkogit ,

It looks to me the function CreateAndGetVirtualNetworkForManagedInstance
was been used by not just one test case.

Please also re-record test cases that referenced this function.

@stefbomdcs
Copy link
Contributor Author

@VeryEarly
Yes, this function is referenced in all MI related test cases.
The tests which are creating MI are very expensive and creation of MI lasts about 4h.
I've tested the change thoroughly for all possible scenarios as it is scoped to adding delegation and NSG object to subnet.
Running all the tests will be very painful and time consuming.

@VeryEarly VeryEarly merged commit a313c79 into Azure:master May 25, 2020
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.

5 participants