-
Notifications
You must be signed in to change notification settings - Fork 561
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rename socket directory to a common name #265
Conversation
Current changes look fine. Are we not handling the helm change for the same in this PR? |
@ShyamsundarR if I change that value I need to make a lot of changes in helm folder, so I kept it as it is, changing this value should be enough for the users to deploy helm chats most of the fields are derived from the values so changing value should be sufficient, @kfox1111 any comments related to this one? |
Just for clarification, I was suggesting to just change that default value in the variable to what is now the default, and NOT make it static and remove it from the variables yaml for Helm. Does that require changes beyond the change to the variable in the file in Helm? |
@Madhu-1 eventhough I see the intention of this PR, I doubt this is really necessary. At times , the container or mount path 'at same location' gives more clarity. If its causing confusion to someone/developer, I think thats still fine/ok to have as they will eventually find out the change should be necessary or not :). |
yes, this is the variable used for binding to the hostpath also. https://github.com/ceph/ceph-csi/blob/csi-v1.0/deploy/rbd/helm/templates/nodeplugin-daemonset.yaml#L109 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine.
@Madhu-1 eventhough I see the intention of this PR, I doubt this is really necessary. At times , the container or mount path 'at same location' gives more clarity. If its causing confusion to someone/developer, I think thats still fine/ok to have as they will eventually find out the change should be necessary or not :). |
Considering I raised this issue, here is what I think,
The existing scheme does not provide any clarity or is not required, IMO simplifying it is better than retaining it with these long names. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a resonable simplification to me without any real negative side effects
@Madhu-1 what about attacher in rbd plugin? |
cc @kfox1111 |
as the socket directory will be created inside the container no need to follow the plugin name in for the directory creation, this will also reduce the code changes if we want to change driver name. Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
rename socket directory to a common name
Syncing latest changes from devel for ceph-csi
as the socket directory will be created inside the container no need to follow the plugin name in for the directory creation, this will also reduce the code changes if we want to change driver name.
Fixes: #223 (comment)
Signed-off-by: Madhu Rajanna madhupr007@gmail.com