Skip to content

Conversation

@vatsalparekh
Copy link
Contributor

Issue: rancher/rancher#48818

Problem

Solution

CheckList

  • Test
  • Docs

@vatsalparekh vatsalparekh force-pushed the k8s-132-update branch 2 times, most recently from 6cfdeb4 to 75b4ecf Compare February 17, 2025 19:56
@vatsalparekh vatsalparekh marked this pull request as ready for review February 17, 2025 19:58
@vatsalparekh vatsalparekh requested a review from a team as a code owner February 17, 2025 19:58
Copy link
Contributor

@tomleb tomleb left a comment

Choose a reason for hiding this comment

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

LGTM, going to assign someone from Collie for the rule resolver changes just in case.

Copy link
Contributor

@tomleb tomleb left a comment

Choose a reason for hiding this comment

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

Bleh, forgot about the kube-version field in the charts. It needs to be bumped as well here: https://github.com/rancher/webhook/blob/main/charts/rancher-webhook/Chart.yaml#L14.

@tomleb tomleb requested a review from JonCrowther February 17, 2025 20:05
@tomleb
Copy link
Contributor

tomleb commented Feb 17, 2025

@JonCrowther added you because we're updating the RuleResolving code, I think that's because the new K8s version added a context.Context to all of these libraries.

Copy link
Contributor

@JonCrowther JonCrowther left a comment

Choose a reason for hiding this comment

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

The RuleResolver stuff looks good to me, but the unused parameters need to be fixed

@vatsalparekh vatsalparekh force-pushed the k8s-132-update branch 3 times, most recently from 4873d47 to a7ac7ff Compare February 18, 2025 07:54
Copy link
Collaborator

@crobby crobby left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Seems like there are a few unused imports that are preventing the tests from running. Everything else seems good.

crobby
crobby previously approved these changes Feb 18, 2025
Signed-off-by: Vatsal Parekh <vatsalparekh@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants