-
Notifications
You must be signed in to change notification settings - Fork 35
[feat] make firewallID mutable, propagate it from lmt->lm #813
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #813 +/- ##
==========================================
+ Coverage 63.20% 63.42% +0.22%
==========================================
Files 71 71
Lines 7359 7410 +51
==========================================
+ Hits 4651 4700 +49
Misses 2435 2435
- Partials 273 275 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| // update the firewallID if needed. | ||
| if !slices.Contains(attachedFirewalls, machineScope.LinodeMachine.Spec.FirewallID) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're replacing an array with an individual ID - is this OK?
d0e4224 to
f767e7f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for mutable FirewallID configurations in LinodeMachineTemplate and propagates firewall changes from LinodeMachineTemplate to LinodeMachine resources. It enables dynamic firewall management for Linode instances.
- Removes immutability constraints on FirewallID fields in both LinodeMachine and LinodeMachineTemplate
- Adds reconciliation logic to propagate firewall changes from templates to machines and update Linode instances
- Introduces new client methods for listing and updating instance firewalls with corresponding mock implementations
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| api/v1alpha2/linodemachine_types.go | Removes immutability validation for FirewallID field and adds documentation |
| api/v1alpha2/linodemachinetemplate_types.go | Adds FirewallID field to LinodeMachineTemplateStatus |
| config/crd/bases/*.yaml | Updates CRD definitions to reflect API changes and remove immutability constraints |
| clients/clients.go | Adds new interface methods for firewall operations |
| internal/controller/linodemachine_controller.go | Implements firewall reconciliation logic for LinodeMachine |
| internal/controller/linodemachinetemplate_controller.go | Adds firewall propagation from template to machines |
| mock/client.go | Adds mock implementations for new firewall client methods |
| observability/wrappers/linodeclient/linodeclient.gen.go | Adds tracing wrappers for new firewall methods |
| go.mod | Updates dependencies |
| docs/src/reference/out.md | Updates documentation with new field descriptions |
| Test files | Adds comprehensive test coverage for firewall functionality |
Comments suppressed due to low confidence (1)
internal/controller/linodemachinetemplate_controller.go:190
- Missing log statement in reconcileFirewallID function. Consider adding a log message similar to the reconcileTags function to track successful firewall ID updates.
return nil
54bd517 to
ec26fb8
Compare
ec26fb8 to
b97d7d4
Compare
61acc39
What this PR does / why we need it:
LinodeMachineTemplate->LinodeMachineWhich issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
TODOs: