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.

Sign up for GitHub

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

Jump to bottom

Add AzureEnvironment to AzureManagedControlPlane #3509

Merged
merged 1 commit into from
May 22, 2023
Merged

Add AzureEnvironment to AzureManagedControlPlane #3509

merged 1 commit into from
May 22, 2023

Conversation

luthermonson
Copy link
Contributor

What type of PR is this?
/kind feature

What this PR does / why we need it:
Adds support for AzureManagedControlPlane.AzureEnvironment allowing a user to set a string value which translates into the configuration passed into the Azure Client. This functionality already exists in AzureCluster and this PR allows for Managed clusters to take advantage of the same feature.

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 #3489

Special notes for your reviewer:
There was an override done to bypass some vmss caching issues for tests. This setup a region URL which doesn't work in govcloud and I refactored newAzureManagedMachinePoolService to accomodate additional ways to skip using regional authorizers based on config.

Release note:

Add AzureEnvironment support to AzureManagedControlPlane

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 2, 2023
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 2, 2023
@luthermonson
Copy link
Contributor Author

@devigned for eyes on the rewrite of his previous PR functionality

@devigned devigned requested a review from mboersma May 2, 2023 17:14
@devigned
Copy link
Contributor

devigned commented May 2, 2023

@mboersma this is the PR we chatted about earlier today.

@luthermonson
Copy link
Contributor Author

i just forgot about the conversions, this is probably done from my perspective when those get checked in

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 2, 2023
@luthermonson luthermonson force-pushed the azenv-aks branch 2 times, most recently from a63c3a9 to 78fc4d7 Compare May 2, 2023 18:58
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 2, 2023
@codecov-commenter
Copy link

codecov-commenter commented May 2, 2023 •
edited

Codecov Report

Patch coverage has no change and project coverage change: +0.20 🎉

Comparison is base (4c0d8c7) 52.59% compared to head (b61303d) 52.80%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3509      +/-   ##
==========================================
+ Coverage   52.59%   52.80%   +0.20%     
==========================================
  Files         182      182              
  Lines       18174    18222      +48     
==========================================
+ Hits         9559     9622      +63     
+ Misses       8077     8060      -17     
- Partials      538      540       +2
Impacted Files Coverage Δ
api/v1beta1/azuremanagedcontrolplane_default.go 92.63% <0.00%> (+53.07%) ⬆️
api/v1beta1/azuremanagedcontrolplane_types.go 20.00% <ø> (ø)
api/v1beta1/azuremanagedcontrolplane_webhook.go 80.17% <0.00%> (-1.75%) ⬇️
api/v1beta1/types_class.go 76.47% <ø> (ø)
azure/scope/managedcontrolplane.go 25.00% <0.00%> (ø)
controllers/azuremanagedmachinepool_reconciler.go 50.58% <0.00%> (-3.17%) ⬇️

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@nojnhuh
Copy link
Contributor

nojnhuh commented May 2, 2023

/assign

@luthermonson
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e

@luthermonson
Copy link
Contributor Author

@nojnhuh any more thoughts on this?

@nojnhuh
Copy link
Contributor

nojnhuh commented May 5, 2023

@luthermonson I'm good to lgtm pending at least one other look at whether or not to default the field.

nawazkh
nawazkh reviewed May 8, 2023
View reviewed changes
Copy link
Member

@nawazkh nawazkh left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together!
I shared some questions around defaulting and validations.

api/v1beta1/azuremanagedcontrolplane_types.go Show resolved Hide resolved
nawazkh
nawazkh reviewed May 8, 2023
View reviewed changes
Copy link
Member

@nawazkh nawazkh left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together!
I shared some questions around defaulting and validations.

@luthermonson luthermonson force-pushed the azenv-aks branch 2 times, most recently from 154d3f0 to b61303d Compare May 10, 2023 19:35
@CecileRobertMichon
Copy link
Contributor

/milestone 1.10

@k8s-ci-robot
Copy link
Contributor

@CecileRobertMichon: The provided milestone is not valid for this repository. Milestones in this repository: [next, v1.10]

Use /milestone clear to clear the milestone.

In response to this:

/milestone 1.10

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.

@CecileRobertMichon
Copy link
Contributor

/milestone v1.10

@k8s-ci-robot k8s-ci-robot added this to the v1.10 milestone May 11, 2023
CecileRobertMichon
CecileRobertMichon reviewed May 12, 2023
View reviewed changes
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment •
edited

Choose a reason for hiding this comment

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

last few minor comments but otherwise lgtm! Thank you @luthermonson

/assing @nojnhuh @nawazkh

@nojnhuh
Copy link
Contributor

nojnhuh commented May 12, 2023

/assign @nojnhuh @nawazkh

@nawazkh
Copy link
Member

nawazkh commented May 16, 2023

Looks good to me pending the nits mentioned by Cecile!
Thank you @luthermonson for putting it together! 🌱

Jont828
Jont828 reviewed May 18, 2023
View reviewed changes
Copy link
Contributor

@Jont828 Jont828 left a comment

Choose a reason for hiding this comment

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

Aisde from Cecile's comments, LGTM.

@luthermonson
Copy link
Contributor Author

everything addressed, let me know if there's anything else I can do.

CecileRobertMichon
CecileRobertMichon reviewed May 22, 2023
View reviewed changes
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 22, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: cb5df934440a28173b97d5aa5767f72fcd3580cc

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 22, 2023
@k8s-ci-robot k8s-ci-robot merged commit bee83d3 into kubernetes-sigs:main May 22, 2023
@CecileRobertMichon CecileRobertMichon mentioned this pull request Jun 8, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CecileRobertMichon CecileRobertMichon CecileRobertMichon left review comments

@nojnhuh nojnhuh nojnhuh left review comments

@Jont828 Jont828 Jont828 left review comments

@nawazkh nawazkh nawazkh left review comments

@sonasingh46 sonasingh46 Awaiting requested review from sonasingh46

@mboersma mboersma Awaiting requested review from mboersma

Assignees

@CecileRobertMichon CecileRobertMichon

@nojnhuh nojnhuh

@nawazkh nawazkh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
CAPZ Planning
Archived in project
Milestone
v1.10
Development

Successfully merging this pull request may close these issues.

AzureEnvironment Support for Managed Clusters
8 participants
@luthermonson @devigned @codecov-commenter @nojnhuh @CecileRobertMichon @k8s-ci-robot @nawazkh @Jont828