✨ Rebase kcp modifications onto v0.16.2 #42
Conversation
kcp-ci-bot
added
dco-signoff: yes
xrstf
force-pushed
the
kcp-1.28
branch
4 times, most recently
from
06ef6f7 to
ffbea9e
Compare
| @@ -201,9 +210,6 @@ type Options struct { | |||
| // ByObject restricts the cache's ListWatch to the desired fields per GVK at the specified object. | |||
| // object, this will fall through to Default* settings. | |||
| ByObject map[client.Object]ByObject | |||
|
|
|||
| // newInformer allows overriding of NewSharedIndexInformer for testing. | |||
| newInformer *func(toolscache.ListerWatcher, runtime.Object, time.Duration, toolscache.Indexers) toolscache.SharedIndexInformer | |||
There was a problem hiding this comment.
Not sure super why this is originally a *func, as regular funcs can already be nil. As our previous fork (kcp-1.26) already introduced NewInformerFunc client.NewInformerFunc, I removed this one here and adjusted the related code to not expect a function pointer.
There was a problem hiding this comment.
is this different func type than client.NewInformerFunc?
| if options.SyncPeriod == nil { | ||
| options.SyncPeriod = opts.SyncPeriod | ||
| } |
There was a problem hiding this comment.
The previous fork had https://github.com/kcp-dev/controller-runtime/blob/kcp-1.26/pkg/kcp/wrappers.go#L192-L200:
if options.Resync == nil {
options.Resync = opts.Resync
}
if options.Namespace == "" {
options.Namespace = opts.Namespace
}
if opts.Resync == nil {
opts.Resync = options.Resync
}I presume it was a typo to both set options.Resync = opts.Resync and opts.Resync = options.Resync, given that opts is then not used within the function anymore.
xrstf
changed the title
| GOBIN=$(abspath $(TOOLS_BIN_DIR)) go install github.com/golangci/golangci-lint/cmd/golangci-lint@$(shell cat .github/workflows/golangci-lint.yml | grep "version: v" | sed 's/.*version: //') | ||
|
|
||
| .PHONY: tools | ||
| tools: $(GO_APIDIFF) $(CONTROLLER_GEN) $(GOLANGCI_LINT) |
There was a problem hiding this comment.
why are we diverting here from upstream?
There was a problem hiding this comment.
I carried these changes over from the kcp-1.26 branch, where the commits just said
UPSTREAM: : make: use go install for lint
UPSTREAM: : make: add tools target
If you want I can remove them and see if something breaks, as I am also not sure why exactly we added these.
There was a problem hiding this comment.
No strong opinion, was just curious.
| - davidfestal | ||
| - ncdc | ||
| - stevekuznetsov | ||
| - sttts |
There was a problem hiding this comment.
We should change this, follow-up or here.
| // NewClusterAwareCache returns a cache.Cache that handles multi-cluster watches. | ||
| func NewClusterAwareCache(config *rest.Config, opts cache.Options) (cache.Cache, error) { | ||
| c := rest.CopyConfig(config) | ||
| c.Host += "/clusters/*" |
There was a problem hiding this comment.
I know this is preexisting. But we must be much more explicit about wildcard vs. non-wildcard watches. This is similar to the namespaced informers. It can well be that one wants multiple cluster support, but only for a handful of workspaces. Also in many cases (basically if you don't talk to a virtual apiserver endpoint) you won't have permission for wildcard requests.
There was a problem hiding this comment.
That describes exactly the issues we recently faced. We really should improve here, as you said.
|
/lgtm |
|
LGTM label has been added. Git tree hash: f5773d5fdb3d8c36ee2d5faaaca1bb854da3bef4
|
|
We can rename the branch to kcp-1.28 and make it the default. |
xrstf
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: sttts 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 |
|
I'll rename the branch once this is merged. |
This PR applies the modifications from the kcp-1.26 branch onto the most recent version (0.16.2), plus it bumps dependencies to Kubernetes 1.28.
Fixes #41