-
Notifications
You must be signed in to change notification settings - Fork 251
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
feat(cephfs): implementation of mkdirs #832
Conversation
@maitredede Please go through the above checklist and make sure that every requirement is met. |
I see some unrelated changes in the pull request. Please make sure that you only touch the relevant API sections. |
Hi @anoopcs9 , thanks for your time 😄 I have gone through all checks, but I don't know if I did well with I have also used |
Hi there, thank you very much for the contribution! Without speaking for @anoopcs9 I think he might have meant to make use of the checkbox feature on github and tick off the boxes so that we know you followed each guideline. For example, I can see that you added a test so it'd be good to tick off that item. Same for documenting the function. One item that's not quite right is the API policy - even though this is a small api and probably an uncontroversial implementation we expect the new api to be added in it's own file and then the go build tag Additionally, we'd prefer if you squashed the patches in the PR into one commit. That way the intermediate changes, like cleaning up the indents in the doc comments, disappear from commit history. Finally, we use a commit message style closer to that of the ceph project or the linux kernel. Just use a subsystem name like |
a1d9149
to
2afaf3b
Compare
Hello @phlogistonjohn, Thanks for your reply. I have followed your instructions and updated my first post accordingly. |
Thank you John, that's exactly what I wanted. @maitredede Thanks for making the required changes. Can you also avoid using the preview suffix from the source file name? makedirs itself holds good. Preview nature of the API is already indicated using the build tags inside which will get removed once it reaches the expected stable version in future. |
2afaf3b
to
bbcb8f2
Compare
Yes, of course, I missed this one 😅 |
bbcb8f2
to
3ef3ae5
Compare
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.
All good.
See below for 2 minor comments.
cephfs/makedirs.go
Outdated
// MakeDirs creates multiple directories at once. | ||
// | ||
// Implements: | ||
// |
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.
Unnecessary empty new line?
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.
Agreed. Please remove this.
@maitredede Can you also add |
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.
Please fix the extra whitespace in the doc comment and add a signed off by line as @anoopcs9 suggested. After that I think we'll be good.
cephfs/makedirs.go
Outdated
// MakeDirs creates multiple directories at once. | ||
// | ||
// Implements: | ||
// |
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.
Agreed. Please remove this.
22f0015
to
aadb632
Compare
Hello :) The whitespace seams to be added by a code formatting tool (I don't know if it is the gofmt used by vscode extension), I will try to double check for my next PRs. The "signed off by" line has been added to my first message. Is is ok ? |
It looks like you pasted it in the PR description. It belongs in the commit message. Try running |
Signed-off-by: Damien <maitredede@gmail.com>
aadb632
to
0f4da87
Compare
Thanks for you help :) |
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.
LGTM. Thank you for working with us to get everything meeting the process!
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.
Hello, this PR adds "ceph_mkdirs" implementation (to create path+subpath at once).
Signed-off-by: Damien DALY maitredede@gmail.com
Checklist
//go:build ceph_preview
make api-update
to record new APIs