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

Common-section-for-Volume-Snapshot #623

Merged
merged 3 commits into from
May 16, 2023

Conversation

mgandharva
Copy link
Collaborator

Description

Created a common section for Volume Snapshot Requirements that all the CSI driver Helm installation reference.

GitHub Issues

List the GitHub issues impacted by this PR:

GitHub Issue #
dell/csm#811

Checklist:

  • Have you run a grammar and spell checks against your submission?
  • Have you tested the changes locally?
  • Have you tested whether the hyperlinks are working properly?
  • Did you add the examples wherever applicable?
  • Have you added high-resolution images?

Copy link

@bharathsreekanth bharathsreekanth left a comment

Choose a reason for hiding this comment

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

See my comment.

```bash
git clone https://github.com/kubernetes-csi/external-snapshotter/
cd ./external-snapshotter
git checkout release-<your-version> ---> Reference the current version.

Choose a reason for hiding this comment

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

What is Reference the current version pointing to ? Is it a link? If we know the specific version, such as 6.2.x then can we not say "git checkout release-6.2.0" in that step?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done


### (Optional) Volume Snapshot Requirements

Applicable only if you decided to enable snapshot feature in `values.yaml`

Choose a reason for hiding this comment

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

Change "decided" to "decide"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link

@rsedlock1958 rsedlock1958 left a comment

Choose a reason for hiding this comment

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

One minor edit required

@mgandharva
Copy link
Collaborator Author

I have done the changes as per the given comments, please let me know if there are any other changes required.

```bash
git clone https://github.com/kubernetes-csi/external-snapshotter/
cd ./external-snapshotter
git checkout release-6.2.0

Choose a reason for hiding this comment

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

Just make sure this is the correct version. I was only referring to having a version here so that users can copy-paste these lines if needed without edits, but I would still confirm if all the teams are using 6.2.0 or another version.

Copy link
Collaborator

@prablr79 prablr79 left a comment

Choose a reason for hiding this comment

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

LGTM

@mgandharva mgandharva merged commit 70583f3 into main May 16, 2023
@mgandharva mgandharva deleted the Common-section-for-Volume-Snapshot branch May 16, 2023 15:01
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