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

feat(cephfs): implementation of mkdirs #832

Merged
merged 1 commit into from
Feb 20, 2023
Merged

Conversation

maitredede
Copy link
Contributor

@maitredede maitredede commented Feb 12, 2023

Hello, this PR adds "ceph_mkdirs" implementation (to create path+subpath at once).

Signed-off-by: Damien DALY maitredede@gmail.com

Checklist

  • [✓] Added tests for features and functional changes
  • [✓] Public functions and types are documented
  • [✓] Standard formatting is applied to Go code
  • [✓] Is this a new API? Added a new file that begins with //go:build ceph_preview
  • [✓] Ran make api-update to record new APIs

@anoopcs9
Copy link
Collaborator

Checklist

* [ ]  Added tests for features and functional changes

* [ ]  Public functions and types are documented

* [ ]  Standard formatting is applied to Go code

* [ ]  Is this a new API? Added a new file that begins with `//go:build ceph_preview`

* [ ]  Ran `make api-update` to record new APIs

New or infrequent contributors may want to review the go-ceph Developer's Guide including the section on how we track API Status and the API Stability Plan.

The go-ceph project uses mergify. View the mergify command guide for information on how to interact with mergify. Add a comment with @Mergifyio rebase to rebase your PR when github indicates that the PR is out of date with the base branch.

@maitredede Please go through the above checklist and make sure that every requirement is met.

@anoopcs9
Copy link
Collaborator

I see some unrelated changes in the pull request. Please make sure that you only touch the relevant API sections.

@maitredede
Copy link
Contributor Author

Hi @anoopcs9 , thanks for your time 😄

I have gone through all checks, but I don't know if I did well with make api-update (it raised an error, I tried to fix).

I have also used make test-docker. Is there something I have missed ?

@phlogistonjohn
Copy link
Collaborator

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 ceph_preview applied (take a look at the linked documentation up in the PR template for more info). If you do this make api-update will record your new API as a preview API in the json & markdown files.

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 cephfs: as the commit topic (prefix) not feat(...).

@maitredede maitredede force-pushed the impl/mkdirs branch 3 times, most recently from a1d9149 to 2afaf3b Compare February 14, 2023 04:09
@maitredede
Copy link
Contributor Author

Hello @phlogistonjohn,

Thanks for your reply. I have followed your instructions and updated my first post accordingly.

@anoopcs9
Copy link
Collaborator

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.

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.

@maitredede
Copy link
Contributor Author

Can you also avoid using the preview suffix from the source file name?

Yes, of course, I missed this one 😅

@phlogistonjohn phlogistonjohn added the API This PR includes a change to the public API of a go-ceph package label Feb 14, 2023
Copy link
Collaborator

@anoopcs9 anoopcs9 left a 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.

// MakeDirs creates multiple directories at once.
//
// Implements:
//
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unnecessary empty new line?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed. Please remove this.

cephfs/makedirs_test.go Show resolved Hide resolved
@anoopcs9
Copy link
Collaborator

@maitredede Can you also add Signed-off-by: in the commit message? Forgot to comment earlier.

Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a 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.

// MakeDirs creates multiple directories at once.
//
// Implements:
//
Copy link
Collaborator

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 maitredede force-pushed the impl/mkdirs branch 2 times, most recently from 22f0015 to aadb632 Compare February 19, 2023 21:28
@maitredede
Copy link
Contributor Author

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.

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 ?

@phlogistonjohn
Copy link
Collaborator

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.

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 git commit --am -s it should update the commit message to contain the Signed-off-by line based on your local git user settings.

Signed-off-by: Damien <maitredede@gmail.com>
@maitredede
Copy link
Contributor Author

It looks like you pasted it in the PR description. It belongs in the commit message. Try running git commit --am -s it should update the commit message to contain the Signed-off-by line based on your local git user settings.

Thanks for you help :)

Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a 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!

Copy link
Collaborator

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

Thank you.

@mergify mergify bot merged commit 4ea99c4 into ceph:master Feb 20, 2023
@maitredede maitredede deleted the impl/mkdirs branch February 20, 2023 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API This PR includes a change to the public API of a go-ceph package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants