-
Notifications
You must be signed in to change notification settings - Fork 559
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
Update relative path to ceph-config.yaml file #3273
Conversation
Thanks @palvarez89 for the contribution 👍 . The DCO check fails due to missing |
@@ -164,7 +164,7 @@ for more information. | |||
**Deploy Ceph configuration ConfigMap for CSI pods:** | |||
|
|||
```bash | |||
kubectl create -f ../example/ceph-config.yaml | |||
kubectl create -f ../../../example/ceph-config.yaml |
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.
Can you explain in the commit message why this is needed? If the file is relative to this document, a single ../
seems sufficient.
Note that other files are referenced without a path. If this gets corrected, other references should be adapted too.
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.
I know is not explicit, but all the previous steps kind of assume that you are on deploy/cephfs/kubernetes
folder, making the current relative path to the example
folder not work.
If this reason is good enough for you, I can update the commit message.
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.
Sure, that's fine with me. It would be great if you can update the commit message and adjust the subject of your commit as well. I recommend doc: update relative path to ceph-config.yaml file
, the doc:
prefix should be valid for the commitlint check. Make sure to add your Signed-off-by
as well.
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.
Thank you for the pointers. I hadn't noticed the CI failures yet.
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.
Thanks, looks good to me. I have added the do not merge label for the time being, so that other PRs don't need to rebase and retest once this gets merged without full e2e testing.
This one is next once the other PR is merged.
Signed-off-by: Pedro Alvarez <[email protected]>
Describe what this PR does
I know is not explicit, but all the previous steps kind of assume that you are on
deploy/cephfs/kubernetes
folder, making the current relative path to theexample
folder not work.