✨ Add workspace paths as annotations to synced objects #15
Conversation
… the callstack On-behalf-of: @SAP [email protected]
On-behalf-of: @SAP [email protected]
kcp-ci-bot
added
release-note
| path := lc.Annotations[kcpcore.LogicalClusterPathAnnotationKey] | ||
| syncContext = syncContext.WithClusterPath(logicalcluster.NewPath(path)) |
There was a problem hiding this comment.
Should we handle the case that this annotation is empty and throw an error? Is this a valid or invalid scenario?
| if err := kcpdevcorev1alpha1.AddToScheme(scheme); err != nil { | ||
| return nil, fmt.Errorf("failed to register scheme %s: %w", kcpdevcorev1alpha1.SchemeGroupVersion, err) | ||
| } |
There was a problem hiding this comment.
Remind me, does adding the scheme require it to be available in the endpoint? Will this fail on setups that don't enable this optional flag?
There was a problem hiding this comment.
To my knowledge, this simply prepares the local client to work with types from the given scheme. This does not change anything about the wire protocol between us and the apiserver. The client will not suddenly do an API discovery and fail if the type doesn't exist.
| // path. This is optional because it requires additional requests to kcp and | ||
| // should only be used if the cluster path is of interest on the | ||
| // service cluster side. | ||
| EnableClusterPaths bool `json:"enableClusterPaths,omitempty"` |
There was a problem hiding this comment.
Nitpick q: Should we be talking about "cluster paths" or "workspace paths" here? Because logical clusters don't really have paths, the path mapping happens through workspaces.
There was a problem hiding this comment.
I'm all yours regarding naming this flag. Within the codebase, I kinda liked the duality of clusterName and clusterPath, both variables have the same length and same prefix and just mhmhmhmhmhmhm chef's kiss, ya know?
There was a problem hiding this comment.
Let's go with workspace path please.
xrstf
changed the title
|
LGTM label has been added. Git tree hash: 081d020ddc01587c0b265b2c1d80a32f739f3b0e
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: embik 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 |
kcp-ci-bot
added
the
approved
Summary
This PR adds a new option to PublishedResources:
enableWorkspacePaths. When set to true, the syncer will put the kcp workspace path as an additional annotation on the synced objects.Related issue(s)
Fixes #11
Release Notes