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

Support for auto upgrade channel #4129

Merged
merged 1 commit into from
Feb 1, 2024
Merged

Support for auto upgrade channel #4129

merged 1 commit into from
Feb 1, 2024

Conversation

LochanRn
Copy link
Member

@LochanRn LochanRn commented Oct 13, 2023 •
edited
Loading

What type of PR is this?
/kind feature

What this PR does / why we need it:

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

Special notes for your reviewer:

  • cherry-pick candidate

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

Support for Auto upgrade channels in AKS.

@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 13, 2023
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 13, 2023
@LochanRn LochanRn force-pushed the support-auto-upgrade-channel branch from fa76c24 to fc1b277 Compare October 13, 2023 21:31
@LochanRn LochanRn changed the title support for auto upgrade channel Support for auto upgrade channel Oct 13, 2023
@LochanRn LochanRn force-pushed the support-auto-upgrade-channel branch 6 times, most recently from edd6a92 to 5b9d86a Compare October 16, 2023 21:30
@codecov
Copy link

codecov bot commented Oct 16, 2023 •
edited
Loading

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (a40be4c) 62.15% compared to head (631a4b2) 62.25%.
Report is 22 commits behind head on main.

Files Patch % Lines
azure/scope/managedcontrolplane.go 31.25% 11 Missing ⚠️
azure/services/agentpools/spec.go 75.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4129      +/-   ##
==========================================
+ Coverage   62.15%   62.25%   +0.09%     
==========================================
  Files         191      192       +1     
  Lines       18849    19007     +158     
==========================================
+ Hits        11715    11832     +117     
- Misses       6484     6522      +38     
- Partials      650      653       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Jont828
Jont828 reviewed Oct 16, 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.

Thanks for the contribution! Here's some feedback so far.

api/v1beta1/azuremanagedcontrolplane_types.go Outdated Show resolved Hide resolved
api/v1beta1/azuremanagedcontrolplane_types.go Outdated Show resolved Hide resolved
api/v1beta1/azuremanagedcontrolplane_types.go Outdated Show resolved Hide resolved
api/v1beta1/azuremanagedcontrolplane_types.go Outdated Show resolved Hide resolved
api/v1beta1/azuremanagedcontrolplane_types.go Outdated Show resolved Hide resolved
azure/scope/managedcontrolplane.go Outdated Show resolved Hide resolved
azure/scope/managedmachinepool.go Outdated Show resolved Hide resolved
azure/services/agentpools/spec.go Outdated Show resolved Hide resolved
azure/services/managedclusters/managedclusters.go Show resolved Hide resolved
util/versions/version.go Outdated Show resolved Hide resolved
@LochanRn LochanRn force-pushed the support-auto-upgrade-channel branch from 5b9d86a to 4be5961 Compare October 17, 2023 18:27
@Jont828 Jont828 self-assigned this Oct 18, 2023
@jackfrancis
Copy link
Contributor

/assign @nojnhuh

@LochanRn LochanRn force-pushed the support-auto-upgrade-channel branch from 4be5961 to 627db39 Compare October 19, 2023 19:53
@LochanRn
Copy link
Member Author

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

2 similar comments
@LochanRn
Copy link
Member Author

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

@LochanRn
Copy link
Member Author

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

@LochanRn LochanRn force-pushed the support-auto-upgrade-channel branch 2 times, most recently from 99bd115 to 97951ac Compare October 27, 2023 04:58
@LochanRn
Copy link
Member Author

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

@nojnhuh
Copy link
Contributor

nojnhuh commented Jan 16, 2024

Since this was approved a long time ago and there have been lots of changes since then:
/approve cancel

@nojnhuh
Copy link
Contributor

nojnhuh commented Jan 16, 2024

Since this was approved a long time ago and there have been lots of changes since then:
/approve cancel

Or maybe this will work:
/remove-approve

@LochanRn I don't mean to suggest this PR is being held up for any reason, I'm just trying to prevent mistakes in the review process to make sure we don't merge this too soon since we don't often deal with approved but un-LGTM'd PRs.

@nojnhuh nojnhuh removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 16, 2024
Jont828
Jont828 reviewed Jan 16, 2024
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.

Looks fine except for a spacing thing and a test failure. It seems like it's due to a linting issue in azure/scope/managedcontrolplane_test.go. Should be good to LGTM after that!

api/v1beta1/types_class.go Show resolved Hide resolved
@mboersma mboersma modified the milestones: v1.13, next, v1.14 Jan 18, 2024
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 20, 2024
@mboersma mboersma added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 25, 2024
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 27, 2024
@LochanRn LochanRn force-pushed the support-auto-upgrade-channel branch from 6811187 to 259ed64 Compare January 27, 2024 08:33
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 27, 2024
@LochanRn
Copy link
Member Author

/test pull-cluster-api-provider-azure-ci-entrypoint

@LochanRn
Copy link
Member Author

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

@LochanRn
Copy link
Member Author

@nojnhuh @Jont828 How do i fix the above errors ?

I ran make test and make generate locally i don't see anymore changes reflecting !

@nojnhuh
Copy link
Contributor

nojnhuh commented Jan 30, 2024

@nojnhuh @Jont828 How do i fix the above errors ?

I ran make test and make generate locally i don't see anymore changes reflecting !

Which errors do you mean? The only errors I see are Netlify and I think it's safe to assume these changes don't affect that.

@LochanRn LochanRn force-pushed the support-auto-upgrade-channel branch from 259ed64 to 631a4b2 Compare January 30, 2024 17:38
nojnhuh
nojnhuh reviewed Jan 30, 2024
View reviewed changes
Copy link
Contributor

@nojnhuh nojnhuh left a comment

Choose a reason for hiding this comment

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

/lgtm

I'll leave the hold to keep this from merging so @Jont828 has a chance to give this one more look.

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

LGTM label has been added.

Git tree hash: a3a1551dea9dcbdab16298af32ddade1b098afeb

@LochanRn
Copy link
Member Author

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

@Jont828
Copy link
Contributor

Jont828 commented Feb 1, 2024

Thank you for your patience on this - I know you've had to rebase this quite a few times. This PR has come a long way!

/lgtm

@nojnhuh
Copy link
Contributor

nojnhuh commented Feb 1, 2024

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 1, 2024
@k8s-ci-robot k8s-ci-robot merged commit c1e35e3 into kubernetes-sigs:main Feb 1, 2024
19 checks passed
@LochanRn
Copy link
Member Author

LochanRn commented Feb 1, 2024

Thank you for your patience on this - I know you've had to rebase this quite a few times. This PR has come a long way!

Thank you for all the valuable review comments :)

@nojnhuh nojnhuh added the area/managedclusters Issues related to managed AKS clusters created through the CAPZ ManagedCluster Type label Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nawazkh nawazkh nawazkh left review comments

@Jont828 Jont828 Jont828 left review comments

@nojnhuh nojnhuh nojnhuh left review comments

@jackfrancis jackfrancis jackfrancis left review comments

@jsturtevant jsturtevant Awaiting requested review from jsturtevant

Assignees

@nojnhuh nojnhuh

@Jont828 Jont828

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/managedclusters Issues related to managed AKS clusters created through the CAPZ ManagedCluster Type 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. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
CAPZ Planning
Archived in project
Milestone
v1.14
Development

Successfully merging this pull request may close these issues.

Support for AKS upgrade channels
9 participants
@LochanRn @jackfrancis @Jont828 @dtzar @k8s-ci-robot @CecileRobertMichon @nojnhuh @nawazkh @mboersma