feat(NetworkACL): Filter Default Rules from Delta #252
Conversation
rushmash91
force-pushed
the
default-nacl
branch
3 times, most recently
from
ddd6d44 to
acb2e30
Compare
|
/retest |
1 similar comment
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: michaelhtm, rushmash91 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 |
pkg/resource/network_acl/hooks.go
Outdated
| ) error { | ||
| if r.ko.Spec.Entries != nil { | ||
| if err := rm.syncEntries(ctx, r, nil); err != nil { | ||
| if desired.ko.Spec.Entries != nil { |
There was a problem hiding this comment.
| if desired.ko.Spec.Entries != nil { | |
| if len(desired.ko.Spec.Entries) > 0 { |
| func customPreCompare( | ||
| delta *ackcompare.Delta, | ||
| a *resource, | ||
| b *resource, | ||
| ) { | ||
| a.ko.Spec.Entries = filterDefaultRules(a.ko.Spec.Entries) | ||
| b.ko.Spec.Entries = filterDefaultRules(b.ko.Spec.Entries) | ||
| } |
There was a problem hiding this comment.
Why do we filter rules both in preCompare and in syncEntries?
There was a problem hiding this comment.
it was only needed during create, removing from sync and moving create to createEntries
pkg/resource/network_acl/hooks.go
Outdated
| // During create latest is nil, just add the entries when sync is called via createEntries | ||
| if latest == nil { | ||
| toAdd = desired.ko.Spec.Entries | ||
| } |
There was a problem hiding this comment.
why at this level of the code? also toAdd is getting elements in L344. Maybe we need an if/else to make sure we're not executing unecessary logic?
There was a problem hiding this comment.
I'll just move out the creation from sync, don't think its a create idea.
rushmash91
force-pushed
the
default-nacl
branch
from
acb2e30 to
2fdddf6
Compare
pkg/resource/network_acl/hooks.go
Outdated
| if len(filteredEntries) > 0 { | ||
| for _, entry := range filteredEntries { | ||
| if err := rm.createEntry(ctx, desired, *entry); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
| if len(filteredEntries) > 0 { | |
| for _, entry := range filteredEntries { | |
| if err := rm.createEntry(ctx, desired, *entry); err != nil { | |
| return err | |
| } | |
| } | |
| } | |
| for _, entry := range filteredEntries { | |
| if err := rm.createEntry(ctx, desired, *entry); err != nil { | |
| return err | |
| } | |
| } |
rushmash91
force-pushed
the
default-nacl
branch
from
2fdddf6 to
ef2640b
Compare
|
/retest |
|
@rushmash91: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
a-hilaly
changed the title
fixes aws-controllers-k8s/community#2374
Description of changes:
createEntriesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.