search

NVIDIA / k8s-device-plugin

NVIDIA device plugin for Kubernetes
Apache License 2.0
2.45k stars 573 forks source link

Remove duplicated deployment yaml file at root #648

Closed ArangoGutierrez closed 1 month ago

klueska commented 1 month ago

This is meant as an example and is pointed to by the README

ArangoGutierrez commented 1 month ago

This is meant as an example and is pointed to by the README

Yup, but the same file lives under deployments/static so I edited the README to reflect that. maybe the PR title is wrong, is not unused, is duplicated.

klueska commented 1 month ago

When first moving the file down to deployments/static, I actually removed it at the top level and updated the documentation as you are doing. This generated lots of complaints though, because people had written their deployment scripts to just deploy the plugin from this script sitting at the top level. As a "fix" for this, the top-level file was created as a symlink to the file under deployments/static so as not to break any scripting that relied on it being at the top-level.

This was all done before we introduced the helm chart though, so maybe it's not a real problem anymore. Back then people didn't have helm as an option so were just applying this file directly. Nowadays I image most people are using helm, so it might not be (as big) of a problem anymore.

ArangoGutierrez commented 1 month ago

When first moving the file down to deployments/static, I actually removed it at the top level and updated the documentation as you are doing. This generated lots of complaints though, because people had written their deployment scripts to just deploy the plugin from this script sitting at the top level. As a "fix" for this, the top-level file was created as a symlink to the file under deployments/static so as not to break any scripting that relied on it being at the top-level.

This was all done before we introduced the helm chart though, so maybe it's not a real problem anymore. Back then people didn't have helm as an option so were just applying this file directly. Nowadays I image most people are using helm, so it might not be (as big) of a problem anymore.

Totally agree, using this file is for a very very rough example/demo use case, doesn't allow users to config anything of value.

My question then goes, do you think this PR is of value, or what should we do with the duplicated file?

klueska commented 1 month ago

Not sure. On one hand, I can see how this cleanup is aesthetically pleasing (it feels strange to have this file at the top level) -- on the other hand, it's not hurting anything being here since one is just a symlink of the other (it's not an actual "duplicate" file)

elezar commented 1 month ago

I think I'm ok to remove this ~file~symlink -- given the specific documentation update and the time that has elapsed since the initial move:

commit 3d0aa307491e294ecd6dec4904f9839c7b1254a7
Author: Kevin Klues <kklues@nvidia.com>
Date:   Wed Jun 24 13:24:55 2020 +0000

    Move the various plugin.yml files to `deployments/static`

    As part of this, keep a symlink to the "primary" plugin.yml file at the
    top level so that it has a nice URL to access it.

    Signed-off-by: Kevin Klues <kklues@nvidia.com>
klueska commented 1 month ago

OK, but you can't just remove the file at the top level -- you will have to move it into deployments/static and remove the symlink there.

klueska commented 1 month ago

Actually, doing a git log on the file, it looks like the symlink was turned into an actual duplicated file by this commit:

commit 630d6713b89bcdc11ca08536b5c7e0684b789b68
Author: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Date:   Tue Jul 11 17:50:43 2023 +0200

    Update HELM charts to acomodate GFD migration into Device Plugin repo

    Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
ArangoGutierrez commented 1 month ago

Actually, doing a git log on the file, it looks like the symlink was turned into an actual duplicated file by this commit:

commit 630d6713b89bcdc11ca08536b5c7e0684b789b68
Author: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Date:   Tue Jul 11 17:50:43 2023 +0200

    Update HELM charts to acomodate GFD migration into Device Plugin repo

    Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>

So it was my bad since the beginning :( , I wonder why it got moved from symlink to file during that operation ... but sorry

klueska commented 1 month ago

Well it gave us the opportunity to have this conversation, so no harm no foul.

I guess we just move ahead with the PR as-is then? I leave the approval up to @elezar.