Skip to content

add function to get object namespaced name#24

Open
Vicente-Cheng wants to merge 2 commits intoharvester:mainfrom
Vicente-Cheng:add-get-namespaced-name
Open

add function to get object namespaced name#24
Vicente-Cheng wants to merge 2 commits intoharvester:mainfrom
Vicente-Cheng:add-get-namespaced-name

Conversation

@Vicente-Cheng
Copy link
Copy Markdown
Collaborator

Add the general function to return the namespaced name.

@Vicente-Cheng Vicente-Cheng requested review from bk201 and votdev March 24, 2025 10:36
@Vicente-Cheng Vicente-Cheng force-pushed the add-get-namespaced-name branch from 4945a87 to 6a42808 Compare March 24, 2025 10:37
@bk201 bk201 requested a review from ibrokethecloud March 25, 2025 00:55
    - we could use this function to generate the <namespace>/<name>

Signed-off-by: Vicente Cheng <vicente.cheng@suse.com>
@Vicente-Cheng Vicente-Cheng force-pushed the add-get-namespaced-name branch from 6a42808 to 03c0193 Compare March 25, 2025 12:08
@Vicente-Cheng Vicente-Cheng requested a review from bk201 March 25, 2025 12:08
innobead
innobead previously approved these changes Mar 25, 2025
func GetNamespacedName[T any](t T) (string, error) {
switch obj := any(t).(type) {
case metav1.Object:
if obj.GetNamespace() == "" {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't an empty string mean the default namespace, per the k8s api doc?

Copy link
Copy Markdown
Member

@votdev votdev Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An empty namespace is equivalent to the "default" namespace, but "default" is the canonical representation. Not all objects are required to be scoped to a namespace - the value of this field for those objects will be empty.

In our case, however, we should assume that we have a namespace, because otherwise the use of this helper function would make no sense. This is also implied by the function name. Therefore I think it is correct to throw an error.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ihcsim for pointing out this.

But I have the same point with @votdev. This is the internal helper function, and it is used for the namespaced object. So, I would like to return an error once the namespace is empty.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only hesitation is that how do we guarantee default is always specified, considering Harvester is interacting with other components like kubevirt and LH. Will those components follow the same convention? Or will this function be used strictly for Harvester resources only?

Signed-off-by: Vicente Cheng <vicente.cheng@suse.com>
@Vicente-Cheng Vicente-Cheng force-pushed the add-get-namespaced-name branch from 3feb537 to caf82de Compare March 27, 2025 17:01
@Vicente-Cheng Vicente-Cheng requested review from ihcsim and votdev March 27, 2025 17:01
@votdev
Copy link
Copy Markdown
Member

votdev commented May 15, 2025

Any progress here? I want to make use of this function in one of my PR. We can somehow reach a consensus?

@ihcsim
Copy link
Copy Markdown
Contributor

ihcsim commented May 15, 2025

If we want to keep this function, we should follow how K8s does it. Here is an example of the equivalence in the k8s src: https://github.com/kubernetes/kubernetes/blob/1ab54ffa6429fb0a3ff0789d4b98bf76bbfddc94/pkg/controller/devicetainteviction/device_taint_eviction.go#L916-L921

These are examples where the empty namespace is specifically used to indicate cluster-scoped resources:

FWIW, is this function truly necessary? Most examples in k8s src just create the struct as-is, and let server-side do the error handling, and decide whether the inputs are acceptable or not.

@Vicente-Cheng
Copy link
Copy Markdown
Collaborator Author

Sorry, I missed this.

After @ihcsim mentioned, I did some research for the namespace generation mechanism.
TL;DR, in most cases, we would have a namespace. However, we still have a chance to obtain the non-namespace object, depending on the stage.

So, I prefer @ihcsim's last comment, we do nothing with non-namespace objects and leave the generic function here (if we prefer to have the common function for most cases) (e.g. https://github.com/kubernetes/kubernetes/blob/1ab54ffa6429fb0a3ff0789d4b98bf76bbfddc94/pkg/controller/devicetainteviction/device_taint_eviction.go#L916-L921)

Or do we just leave each component to implement what they want?
cc @votdev, @ihcsim

@ihcsim
Copy link
Copy Markdown
Contributor

ihcsim commented May 16, 2025

I am fine with having this generic function, but following the K8s src. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants