-
Notifications
You must be signed in to change notification settings - Fork 430
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
use existing virtualNetwork from a different rg #4606
Conversation
Skipping CI for Draft Pull Request. |
334402f
to
131ccab
Compare
/cc @kubernetes-sigs/cluster-api-provider-azure-pms |
131ccab
to
fe8fd71
Compare
/retest |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4606 +/- ##
==========================================
+ Coverage 62.47% 62.71% +0.23%
==========================================
Files 192 192
Lines 15453 15487 +34
==========================================
+ Hits 9655 9713 +58
+ Misses 5131 5107 -24
Partials 667 667 ☔ View full report in Codecov by Sentry. |
fe8fd71
to
ddc8272
Compare
ddc8272
to
2bbf479
Compare
20c05ee
to
6a3044e
Compare
LGTM label has been added. Git tree hash: aca2741938b9e39dac96f2050d2d01a40f1483f4
|
- check vnets for azure managed control plane templates - add tests to GroupSpecs - update webhooks and add webhook tests - add AzureName to aso groups - use AzureName when rgs are different - normalize Azure name to K8s name - propogate changes to ClusterScope
285e153
to
7c5df96
Compare
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.
/lgtm
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nojnhuh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@willie-yao @nojnhuh could you investigate if the approach here could potentially solve for #4704 |
... and maybe #4699 |
@jackfrancis I don't think this approach will work because the error message is coming from k8s itself regarding the |
@nojnhuh @willie-yao @mboersma can we get a consensus vote on whether or not to classify this as a bug? Was the BYO VNET story always meant (and documented) to support a VNET from a different resource group than the cluster resource group? If so, I think this is a bug fix and can be backported. Thoughts? |
I'd say it counts as a bug since it fixes something that used to work before a certain version and I'm in favor of the backport. |
I think this is fair to backport. |
Yes, this seems worthy of backporting. /cherrypick release-1.14 |
@mboersma: #4606 failed to apply on top of branch "release-1.14":
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@mboersma I can help manually backport this! |
/cherrypick release-1.13 |
Oh wait I got confused. This commit is already in release-1.14 😆 |
@willie-yao: #4606 failed to apply on top of branch "release-1.13":
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR will enable using existing virtualNetwork from a different rg.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #4595
Special notes for your reviewer:
TODOs:
Release note: