Skip to content

Conversation

@slaupster
Copy link
Contributor

@slaupster slaupster commented Jun 10, 2025

the Deployment already has this guard. It should be extended to include its IAM and service.

the `Deployment` already has this guard. It should be extended to include its IAM and service.
@romanbaron
Copy link
Collaborator

Thanks for suggesting this change, it makes a lot of sense.
The reason the validation test fails is because nodescaleadjuster ClusterRole is generated here:
https://github.com/NVIDIA/KAI-Scheduler/blob/main/Makefile#L65
And the generated version doesn't have the if guard that you added.

@slaupster
Copy link
Contributor Author

Thanks for suggesting this change, it makes a lot of sense. The reason the validation test fails is because nodescaleadjuster ClusterRole is generated here: https://github.com/NVIDIA/KAI-Scheduler/blob/main/Makefile#L65 And the generated version doesn't have the if guard that you added.

I have to say I'm not following what the manifests target is intending to do - just generate what is missing ? If I just remove it like last commit we accept going forward none of it is generated because it has go template for the helm ?

@romanbaron
Copy link
Collaborator

All ClusterRoles yaml files are generated from kubebuilder annotations, it assists with keeping track of where the resources in the ClusterRole are actually being used. Here is an example:

// +kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;watch

So these files cannot be manually modified. In addition 'make validate' step in CI attempts to catch a possible case where someone added a kubebuilder annotation but did not re-generate the yaml files.
Usually yaml files can be patched with Kustomize (example: Kustomize) but I am not sure if there is a way to wrap the entire file with a condition using this method.

@github-actions github-actions bot added the stale label Oct 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants