Skip to content

Conversation

mboersma
Copy link
Contributor

@mboersma mboersma commented Aug 6, 2024

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Updates CAPI to v1.8.1 and all that entails.

Which issue(s) this PR fixes:

Fixes #4991

Special notes for your reviewer:

See the migration guide for more information.

  • cherry-pick candidate

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

Bump CAPI to v1.8.1

@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/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 6, 2024
@mboersma
Copy link
Contributor Author

mboersma commented Aug 6, 2024

/retitle [WIP] Bump CAPI to v1.8.0-rc.0

Vendoring changes here are giving me fits. I could use another pair of eyes, or ideas if anyone has any.

@k8s-ci-robot k8s-ci-robot changed the title Bump CAPI to v1.8.0-rc.0 [WIP] Bump CAPI to v1.8.0-rc.0 Aug 6, 2024
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 6, 2024
@troy0820
Copy link
Member

troy0820 commented Aug 6, 2024

Vendoring changes here are giving me fits. I could use another pair of eyes, or ideas if anyone has any.

It looks like you have an issue here:
sigs.k8s.io/controller-runtime => sigs.k8s.io/controller-runtime v0.17.3

That's one of the issues at least.

@mboersma
Copy link
Contributor Author

mboersma commented Aug 6, 2024

It is one of the issues indeed, thanks @troy0820.

I started removing those replace lines locally, but it's a slippery slope and obscure compilation errors continue to accumulate. But that one in particular does need to go, we can start there.

@jackfrancis
Copy link
Contributor

@mboersma just curious, is this PR making this more difficult? #4973

@troy0820
Copy link
Member

troy0820 commented Aug 6, 2024

It's your replaces at the end of the go.mod. The older k8s.io/* deps are clashing with what is imported by controller-runtime and cluster-api.

It is one of the issues indeed, thanks @troy0820.
I started removing those replace lines locally, but it's a slippery slope and obscure compilation errors continue to accumulate. But that one in particular does need to go, we can start there.

Where I am after commenting out all the old replaces was issues with:
image

in controllers/azurejson_machinetemplate_controller.go I can provide diff or make a PR to this branch if I do it fast enough.

@mboersma
Copy link
Contributor Author

mboersma commented Aug 6, 2024

is this PR making this more difficult? #4973

Somewhat, but there's a clear comment that those replaces may need to go away with this change. I was narrowing in on which ones need to remain, but got muddled every time I got close. @nojnhuh may have found the recipe, I merged his changes just now.

@troy0820
Copy link
Member

troy0820 commented Aug 6, 2024

@nojnhuh was faster than I was, but if you want to keep parity with upstream capi, this may be the release you ditch the replaces because I believe cr is running k8s.io/* at v0.30.0 starting in v0.18.0 which this uses. some apis are gone from that (none that we need to worry about) but things changing in controller-runtime/capi may force your hand.

@mboersma mboersma changed the title [WIP] Bump CAPI to v1.8.0-rc.0 [WIP] Bump CAPI to v1.8.0-rc.1 Aug 6, 2024
@nojnhuh
Copy link
Contributor

nojnhuh commented Aug 7, 2024

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

@nojnhuh
Copy link
Contributor

nojnhuh commented Aug 7, 2024

The AKS failure seems 100% consistent when I run the tests locally, but I can't reproduce with the same template using Tilt. CAPZ gets stuck in a panic loop because the spec.controlPlaneRef on the Cluster isn't set after it's created from a ClusterClass. Only the AKS ClusterClass test seems affected, not the self-managed ClusterClass test.

Copy link

codecov bot commented Aug 7, 2024

Codecov Report

Attention: Patch coverage is 0% with 19 lines in your changes missing coverage. Please review.

Project coverage is 51.24%. Comparing base (2e3a57b) to head (57b3d4c).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...ontrollers/azurejson_machinetemplate_controller.go 0.00% 10 Missing ⚠️
main.go 0.00% 7 Missing ⚠️
...ntrollers/azureasomanagedmachinepool_controller.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5057      +/-   ##
==========================================
- Coverage   51.26%   51.24%   -0.02%     
==========================================
  Files         273      273              
  Lines       24648    24650       +2     
==========================================
- Hits        12635    12633       -2     
- Misses      11227    11231       +4     
  Partials      786      786              

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

@nojnhuh
Copy link
Contributor

nojnhuh commented Aug 7, 2024

This may fix the AKS clusterclass test somehow? #5058

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 15, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2024
@mboersma
Copy link
Contributor Author

/retitle Bump CAPI to v1.8.1

@k8s-ci-robot k8s-ci-robot changed the title [WIP] Bump CAPI to v1.8.0-rc.1 Bump CAPI to v1.8.1 Aug 23, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 23, 2024
@mboersma mboersma requested review from nojnhuh and removed request for Jont828 August 23, 2024 16:37
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
/assign @willie-yao

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

LGTM label has been added.

Git tree hash: 203312dbfbc37e1904a098e05ecfdca732c96e09

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 2024
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 2024
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
/approve

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

LGTM label has been added.

Git tree hash: 4f958e7e5671a69405123128286202b27c01bc5b

@k8s-ci-robot
Copy link
Contributor

[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 /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 Aug 30, 2024
@willie-yao
Copy link
Contributor

/retest

@willie-yao
Copy link
Contributor

Error: failed to get provider components for the "cluster-api:v1.7.5" provider: failed to read "components.yaml" from provider's repository "cluster-api": failed to read file "/logs/artifacts/repository/cluster-api/v1.7.5/components.yaml" from local release v1.7.5: stat /logs/artifacts/repository/cluster-api/v1.7.5/components.yaml: no such file or directory

I think you might need to change some of the paths in azure-dev.yaml back to 1.7.5

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 3, 2024
@k8s-ci-robot k8s-ci-robot requested a review from nojnhuh September 3, 2024 15:20
@nojnhuh
Copy link
Contributor

nojnhuh commented Sep 3, 2024

/lgtm

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

LGTM label has been added.

Git tree hash: 023e66e3daba0d09e5eb6ec46e10d87dc3fd29f9

@k8s-ci-robot k8s-ci-robot merged commit 420c741 into kubernetes-sigs:main Sep 3, 2024
21 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Sep 3, 2024
@mboersma mboersma deleted the capi-one-eight-oh branch September 3, 2024 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

CAPI v1.8.0-beta.0 has been released and is ready for testing
6 participants