Skip to content
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

deploy: add cephfs/CSIDriver and cephfs/csi-config-map to API #3837

Merged
merged 2 commits into from
May 25, 2023
Merged

deploy: add cephfs/CSIDriver and cephfs/csi-config-map to API #3837

merged 2 commits into from
May 25, 2023

Conversation

riya-singhal31
Copy link
Contributor

@riya-singhal31 riya-singhal31 commented May 16, 2023

Move cephfs/csidriver and cephfs/csi-config-map to API

updates: #3838

@Rakshith-R
Copy link
Contributor

Move cephfs/csidriver and cephfs/csi-config-map to API

updates: https://issues.redhat.com/browse/RHSTOR-4539

@riya-singhal31
That is downstream link.
Please open an upstream issue with appropriate description and link that here.

@riya-singhal31
Copy link
Contributor Author

Move cephfs/csidriver and cephfs/csi-config-map to API
updates: https://issues.redhat.com/browse/RHSTOR-4539

@riya-singhal31 That is downstream link. Please open an upstream issue with appropriate description and link that here.

Thanks Rakshith, updated.

@nixpanic nixpanic requested a review from a team May 23, 2023 16:11

// NewCSIConfigMap takes a name from the CSIConfigMapValues struct and relaces
// the value in the template. A ConfigMap object is returned which can be
// created in the Kubernetes cluster.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@riya-singhal31 @nixpanic, what is the value to the end user with this one, we dont have any struct keys inside the configmap as expected by CSI, and we are just returning an empty template for the user, which is of significantly less use.

Copy link
Member

Choose a reason for hiding this comment

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

if keys are required, then those should probably be documented better. The current example configmap is empty as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes it's empty but as you mentioned it's documented and we dont have any documentation for the APIs; this is just returning an empty configmap with a name not sure how much helpful it is.

Copy link
Member

Choose a reason for hiding this comment

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

The documentation is missing the in the yaml file itself. yaml can have comments, and ideally has them for required options.

If values are required before deploying, the API should offer a way to set them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

They are required post-deployment but I am still not sure. Just exposing the empty configmap to the user and how it will be useful.

Copy link
Collaborator

Choose a reason for hiding this comment

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

approved for now.


require.NoError(t, err)
require.NotEqual(t, "", yaml)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

new line is missing at the end of the file

@Madhu-1 Madhu-1 added the ok-to-test Label to trigger E2E tests label May 25, 2023
@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.24

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.25

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.26

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.27

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.24

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.25

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.26

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.27

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.24

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.25

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.26

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.27

@github-actions
Copy link

/test ci/centos/upgrade-tests-cephfs

@github-actions
Copy link

/test ci/centos/upgrade-tests-rbd

@github-actions github-actions bot removed the ok-to-test Label to trigger E2E tests label May 25, 2023
Signed-off-by: riya-singhal31 <rsinghal@redhat.com>
Signed-off-by: riya-singhal31 <rsinghal@redhat.com>
@mergify mergify bot added the ok-to-test Label to trigger E2E tests label May 25, 2023
@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.24

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.25

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.26

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.27

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.24

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.25

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.26

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.27

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.24

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.25

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.26

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.27

@github-actions
Copy link

/test ci/centos/upgrade-tests-cephfs

@github-actions
Copy link

/test ci/centos/upgrade-tests-rbd

@github-actions github-actions bot removed the ok-to-test Label to trigger E2E tests label May 25, 2023
@mergify mergify bot merged commit 54fa028 into ceph:devel May 25, 2023
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.

4 participants