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.

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

Update controller-runtime to v0.14.4 #78

Merged
merged 1 commit into from
Feb 10, 2023

Conversation

g-gaston
Copy link
Contributor

@g-gaston g-gaston commented Feb 8, 2023

Description

controller-runtime deprecated the envtest/printer package in v0.14.0. They don't needed anymore since they upgraded to ginko v2. In order to keep the scope contained, I just copied the code we needed, it's quite simple. This allow us to maintain the same version of ginko while bumping controller-runtime.

The fake client provided by controller-runtime was updated to allow for field selectors in List operations. This requires to define the index prior to its use. Since the Job controller uses ".metadata.controller" for a List query, we now set this index in the tests client.
kubernetes-sigs/controller-runtime#2025

Why is this needed

It allows consumers of rufio as a module to start using new versions of controller-runtime. Technically we didn't need to upgrade controller-runtime, just stop using the deprecated package. But I thought it wouldn't hurt, specially once we decide to start using go 1.19, since this comes with versions of k8s.io packages that already support to 1.19.

How Has This Been Tested?

Not tested. Looking at the release notes, it seems like there aren't any behavior changes, so as long as unit tests pass, it should be a transparent drop-in change.

How are existing users impacted? What migration steps/scripts do we need?

No upgrade/migration needed.

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

@chrisdoherty4
Copy link
Member

Thanks, @g-gaston. It looks like we're swallowing the error at https://github.com/tinkerbell/rufio/blob/main/controllers/job_controller.go#L116 that's causing the unit tests to fail.

@g-gaston g-gaston force-pushed the update-controller-runtime branch 2 times, most recently from 147cdc8 to 19d53f4 Compare February 9, 2023 21:08
controller-runtime deprecated the envtest/printer package in v0.14.0. They don't
needed anymore since they upgraded to ginko v2. In order to keep the
scope contained, I just copied the code we needed, it's quite simple.
This allow us to maintain the same version of ginko while bumping
controller-runtime.

The fake client provided by controller-runtime was updated to allow
for field selectors in List operations. This requires to define the
index prior to its use. Since the Job controller uses
".metadata.controller" for a List query, we now set this index in the
tests client.
kubernetes-sigs/controller-runtime#2025

Signed-off-by: Guillermo Gaston <gaslor@amazon.com>
@codecov
Copy link

codecov bot commented Feb 9, 2023

Codecov Report

Merging #78 (267257e) into main (dee5b26) will increase coverage by 1.24%.
The diff coverage is 33.33%.

❗ Current head 267257e differs from pull request most recent head 4f25b88. Consider uploading reports for the commit 4f25b88 to get more accurate results

@@            Coverage Diff             @@
##             main      #78      +/-   ##
==========================================
+ Coverage   59.50%   60.74%   +1.24%     
==========================================
  Files           4        4              
  Lines         321      321              
==========================================
+ Hits          191      195       +4     
+ Misses         97       90       -7     
- Partials       33       36       +3     
Impacted Files Coverage Δ
controllers/job_controller.go 55.64% <33.33%> (+3.22%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@jacobweinstock jacobweinstock left a comment

Choose a reason for hiding this comment

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

Thanks @g-gaston!

@chrisdoherty4 chrisdoherty4 added the ready-to-merge Mergify: Ready for Merging label Feb 10, 2023
@chrisdoherty4 chrisdoherty4 merged commit 50c4ce3 into tinkerbell:main Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Mergify: Ready for Merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants