-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Option to customize NamespaceTransformer overwrite behaviour #4708
Option to customize NamespaceTransformer overwrite behaviour #4708
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: KnVerey 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 |
@natasha41575 I split this out of #4704 for ease of review, and because the role binding case has less code but more to discuss (i.e. this: #4704 (comment)). This one is ready for review. |
api/filters/namespace/namespace.go
Outdated
@@ -19,6 +19,9 @@ type Filter struct { | |||
// FsSlice contains the FieldSpecs to locate the namespace field | |||
FsSlice types.FsSlice `json:"fieldSpecs,omitempty" yaml:"fieldSpecs,omitempty"` | |||
|
|||
// SkipExisting means only blank namespace fields will be set | |||
SkipExisting bool `json:"skipExisting" yaml:"skipExisting"` |
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.
Naming suggestions welcome! ( I want it to have negative polarity though, so false when missing give the current default behaviour)
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.
An alternative idea I had was EmptyOnly
or BlankOnly
- it doesn't reverse the polarity but reverses the wording to focus on the fact that this will only set empty/blank namespace fields. I like this slightly better because it highlights what does get changed, rather than what is skipped.
I'll defer to what you choose though, I think either way is fine.
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.
I like EmptyOnly
. Another option could be UnsetOnly
, which are more specific to namespace scoped resources. This resource are set to "default" in deployment.
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.
👍 I like UnsetOnly
.
@@ -37,7 +37,7 @@ linters: | |||
- gocyclo | |||
# - godot | |||
# - godox | |||
- goerr113 | |||
# - goerr113 |
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.
I'm glad we're disabling this, it's been slightly annoying to work with
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.
Feel free to question the set of linters I enabled when they really don't align with the style in this codebase and/or value is low. I erred on the side of enabling more of them, without necessarily having a clear picture of the implications.
api/filters/filtersutil/setters.go
Outdated
return false | ||
} | ||
if key == "" { | ||
// scalar node |
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.
nit: Would it be worth adding a check here that the node is actually a scalar node?
fn := filtersutil.TrackableSetter{}.SetEntryIfEmpty("foo", "false", yaml.NodeTagBool) | ||
rn := yaml.NewListRNode("nope") | ||
rn.AppendToFieldPath("dummy", "path") | ||
assert.EqualError(t, fn(rn), "wrong Node Kind for dummy.path expected: MappingNode was SequenceNode: value: {- nope}") |
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.
This is outside the scope of this PR, but I hope we can improve this error message some day
api/filters/nameref/nameref.go
Outdated
@@ -337,19 +337,16 @@ func (f Filter) selectReferral( | |||
return candidates[0], nil | |||
} | |||
ids := getIds(candidates) | |||
f.failureDetails(candidates) | |||
return nil, fmt.Errorf(" found multiple possible referrals: %s", ids) | |||
return nil, fmt.Errorf(" found multiple possible referrals: %s\n%s", ids, f.failureDetails(candidates)) |
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.
Why is there an extra space at the beginning of this error?
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.
I dunno... there was before too. Probably just a typo.
api/filters/namespace/namespace.go
Outdated
case roleBindingKind, clusterRoleBindingKind: | ||
return true | ||
default: | ||
return false |
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.
the switch statement seems a bit overkill for this check, couldn't we simply return gvk.Kind == roleBindingKind || gvk.Kind == clusterRoleBindingKind
?
@KnVerey: This PR has multiple commits, and the default merge method is: merge. 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. |
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.
apart from the lint error, this LGTM
api/filters/namespace/namespace.go
Outdated
// SkipExisting means only blank namespace fields will be set | ||
SkipExisting bool `json:"skipExisting" yaml:"skipExisting"` | ||
// UnsetOnly means only blank namespace fields will be set | ||
UnsetOnly bool `json:"UnsetOnly" yaml:"UnsetOnly"` |
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.
should be lowercase
UnsetOnly bool `json:"unsetOnly" yaml:"unsetOnly"`
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.
🤦♀️
f2b1438
to
de7b103
Compare
/lgtm |
Resolves #880 (the no. 2 issue by upvotes as of today)
Split out of #4704, which will be rebased on top of this
We can't change the default, since that would be enormously disruptive to our users. Instead, this PR proposes a new option for the transformer to enable it to behave in the desired manner. This option are exposed explicitly via the transformer's configuration resource, which can be used in the
transformers
field. While using resource-specific annotations to configure transformers is an interesting idea that may be appropriate in some cases, my opinion is that transformer-level is better granularity for these particular options. I also considered a selection-base solution as suggested here, but at that point I think folks should reach for replacements instead, now that they exist.