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

refactor: controller-runtime v0.16.3 #246

Merged
merged 1 commit into from
Nov 7, 2023
Merged

refactor: controller-runtime v0.16.3 #246

merged 1 commit into from
Nov 7, 2023

Conversation

KevFan
Copy link
Contributor

@KevFan KevFan commented Aug 31, 2023 •
edited
Loading

Resolves: #237

@KevFan KevFan added kind/enhancement New feature or request good first issue Good for newcomers status/blocked Blocked by something... size/small area/dependencies Changes dependencies labels Aug 31, 2023
@KevFan
Copy link
Contributor Author

KevFan commented Aug 31, 2023 •
edited
Loading

Blocked / Kept as draft for now as we've decided to wait for MVP release before making these changes

@codecov
Copy link

codecov bot commented Aug 31, 2023 •
edited
Loading

Codecov Report

Merging #246 (9a839ac) into main (12f785e) will decrease coverage by 0.40%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #246      +/-   ##
==========================================
- Coverage   64.79%   64.39%   -0.40%     
==========================================
  Files          35       35              
  Lines        3806     3806              
==========================================
- Hits         2466     2451      -15     
- Misses       1148     1159      +11     
- Partials      192      196       +4
Flag Coverage Δ
integration 69.79% <100.00%> (-0.74%) ⬇️
unit 58.20% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
api/v1beta1 (u) ∅ <ø> (∅)
pkg/common (u) 73.92% <ø> (ø)
pkg/istio (u) 30.24% <ø> (ø)
pkg/log (u) 31.81% <ø> (ø)
pkg/reconcilers (u) 33.68% <ø> (ø)
pkg/rlptools (u) 56.41% <ø> (ø)
controllers (i) 69.79% <100.00%> (-0.74%) ⬇️
Files Coverage Δ
controllers/authpolicy_controller.go 71.62% <100.00%> (ø)
controllers/gateway_eventmapper.go 80.95% <100.00%> (ø)
controllers/httproute_eventmapper.go 100.00% <100.00%> (ø)
controllers/ratelimitpolicy_controller.go 69.40% <100.00%> (-2.24%) ⬇️

... and 4 files with indirect coverage changes

KevFan
KevFan commented Aug 31, 2023
View reviewed changes
main.go
options := ctrl.Options{Scheme: scheme}

if configFile != "" {
options, err = options.AndFrom(ctrl.ConfigFile().AtPath(configFile))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ComponentConfig has been deprecated (https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.15.0). We can either use a custom implementation that sets Manager.Options directly or move back to configuration via arguments.

Moving back to configuration using arguments as it's the easier option and we do not have many configurations in general

@KevFan KevFan force-pushed the controller-runtime branch 2 times, most recently from 5b415d4 to b1331a4 Compare September 6, 2023 13:18
@KevFan KevFan mentioned this pull request Sep 8, 2023
Closed
@KevFan KevFan changed the title refactor: controller-runtime v0.16.0 refactor: controller-runtime v0.16.1 Sep 19, 2023
@KevFan KevFan force-pushed the controller-runtime branch 2 times, most recently from ea1019e to a41e652 Compare October 3, 2023 16:38
@KevFan KevFan force-pushed the controller-runtime branch 3 times, most recently from 497c187 to e73c4c8 Compare October 26, 2023 10:06
@KevFan KevFan changed the title refactor: controller-runtime v0.16.1 refactor: controller-runtime v0.16.2 Oct 26, 2023
@KevFan KevFan removed the status/blocked Blocked by something... label Oct 31, 2023
@KevFan
Copy link
Contributor Author

KevFan commented Oct 31, 2023

MVP I believe has been completed, so this is ready for review

@KevFan KevFan marked this pull request as ready for review October 31, 2023 14:34
@KevFan KevFan requested a review from a team as a code owner October 31, 2023 14:34
@KevFan KevFan changed the title refactor: controller-runtime v0.16.2 refactor: controller-runtime v0.16.3 Nov 1, 2023
@KevFan
Copy link
Contributor Author

KevFan commented Nov 1, 2023

Gateway api v1.0.0 has been released using controller runtime v0.16.3

@adam-cattermole adam-cattermole mentioned this pull request Nov 1, 2023
Merged
1 task
adam-cattermole
adam-cattermole approved these changes Nov 1, 2023
View reviewed changes
Copy link
Member

@adam-cattermole adam-cattermole left a comment

Choose a reason for hiding this comment

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

These changes look good - I opted for a near identical implementation when I started to look at gwapi v1 and needed a newer controller-runtime. Nice work!

@KevFan
Copy link
Contributor Author

KevFan commented Nov 3, 2023

Rebased to resolve conflicts with main

eguzki
eguzki reviewed Nov 6, 2023
View reviewed changes
main.go
"The operator will load its initial configuration from this file. "+
"Omit this flag to use the default configuration values. "+
"Command-line flags override configuration from this file.")
flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.")
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the deployment of the operator should change after this change. Or defaults are good enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think these defaults are good. These are the same as 52b2cdb which was before it was moved to loading config using file.

Only difference I see between defaults and deployment options is that leacterElection is true only for deployment but false by default 🤔 :

Copy link
Contributor

Choose a reason for hiding this comment

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

But the deployment is setting it explicitly, so we should be good.

@eguzki
Copy link
Contributor

eguzki commented Nov 6, 2023 •
edited
Loading

LGTM

however, git is yelling at conflicting files. On fixing them, happy to approve it

@KevFan KevFan merged commit 8943f5d into Kuadrant:main Nov 7, 2023
15 checks passed
This was referenced Nov 8, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eguzki eguzki eguzki approved these changes

@adam-cattermole adam-cattermole adam-cattermole approved these changes

Assignees
No one assigned
Labels
area/dependencies Changes dependencies good first issue Good for newcomers kind/enhancement New feature or request size/small
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

dep: upgrade controller runtime
3 participants
@KevFan @eguzki @adam-cattermole