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

Add coding.md into docs #304

Merged
merged 3 commits into from
Jun 4, 2019
Merged

Add coding.md into docs #304

merged 3 commits into from
Jun 4, 2019

Conversation

humblec
Copy link
Collaborator

@humblec humblec commented Apr 4, 2019

Signed-off-by: Humble Chirammal hchiramm@redhat.com

@humblec
Copy link
Collaborator Author

humblec commented Apr 4, 2019

@gman0 @Madhu-1 @rootfs PTAL

Signed-off-by: Humble Chirammal <hchiramm@redhat.com>
Signed-off-by: Humble Chirammal <hchiramm@redhat.com>
@humblec humblec force-pushed the contributing branch 2 times, most recently from 807873e to 4607e94 Compare April 4, 2019 06:59
README.md Outdated
@@ -20,6 +20,12 @@ CephFS CSI plugins, see documentation in `docs/`.

For example usage of RBD and CephFS CSI plugins, see examples in `examples/`.

## Contributing to this repo

Please follow [contibution guide](https://github.com/ceph/ceph-csi/tree/csi-v1.0/docs/coding.md)
Copy link
Collaborator

Choose a reason for hiding this comment

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

csi-v1.0 or master?

README.md Outdated
## Contributing to this repo

Please follow [contibution guide](https://github.com/ceph/ceph-csi/tree/csi-v1.0/docs/coding.md)
and [development-guide](https://github.com/ceph/ceph-csi/tree/csi-v1.0/docs/development-guide.md)
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

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


### Workspace and repository setup

1. [Download](https://golang.org/dl/) Go (>=1.9) and [install](https://golang.org/doc/install) it on your system.
Copy link
Collaborator

Choose a reason for hiding this comment

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

go version need to be 1.11.x or greater

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.

Run the test suite, which includes linting checks, static code check, and unit
tests:

`$ make tests`
Copy link
Collaborator

Choose a reason for hiding this comment

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

make test

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

@humblec humblec force-pushed the contributing branch 3 times, most recently from 9705b42 to 91b85e8 Compare April 4, 2019 07:05
@humblec
Copy link
Collaborator Author

humblec commented Apr 4, 2019

@Madhu-1 addressed the comments. Thanks!

Copy link
Collaborator

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

just a small nit is required

@humblec do we need to mention that we have different branches for csi-v1.0.0 and csi-v0.3.0

docs/coding.md Outdated
```
<import standard library packages>

<import ceph packages>
Copy link
Collaborator

Choose a reason for hiding this comment

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

ceph to ceph-csi

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Apr 4, 2019

@humblec CI failure PTAL

$ scripts/lint-text.sh --require-all
mdl --style scripts/mdl-style.rb ./README.md 
mdl --style scripts/mdl-style.rb ./docs/coding.md 
./docs/coding.md:13: MD001 Header levels should only increment by one level at a time
./docs/coding.md:45: MD010 Hard tabs
./docs/coding.md:46: MD010 Hard tabs
./docs/coding.md:13: MD022 Headers should be surrounded by blank lines
./docs/coding.md:14: MD032 Lists should be surrounded by blank lines

mdl --style scripts/mdl-style.rb ./docs/development-guide.md 
./docs/development-guide.md:1: MD002 First header should be a top level header
./docs/development-guide.md:55: MD009 Trailing spaces
./docs/development-guide.md:2: MD013 Line length
./docs/development-guide.md:12: MD013 Line length
./docs/development-guide.md:13: MD013 Line length
./docs/development-guide.md:15: MD013 Line length
./docs/development-guide.md:20: MD013 Line length
./docs/development-guide.md:31: MD013 Line length
./docs/development-guide.md:35: MD013 Line length
./docs/development-guide.md:56: MD013 Line length
./docs/development-guide.md:58: MD013 Line length
./docs/development-guide.md:61: MD013 Line length
./docs/development-guide.md:63: MD013 Line length
./docs/development-guide.md:64: MD013 Line length
./docs/development-guide.md:65: MD013 Line length
./docs/development-guide.md:68: MD013 Line length
./docs/development-guide.md:1: MD022 Headers should be surrounded by blank lines
./docs/development-guide.md:1: MD026 Trailing punctuation in header
./docs/development-guide.md:20: MD027 Multiple spaces after blockquote symbol
./docs/development-guide.md:38: MD029 Ordered list item prefix
./docs/development-guide.md:39: MD029 Ordered list item prefix
./docs/development-guide.md:62: MD032 Lists should be surrounded by blank lines

@humblec
Copy link
Collaborator Author

humblec commented Apr 4, 2019

@Madhu-1 sure. will be on it soon.

@humblec
Copy link
Collaborator Author

humblec commented Apr 4, 2019

@humblec do we need to mention that we have different branches for csi-v1.0.0 and csi-v0.3.0

I think we can skip this for now.

docs/coding.md Outdated
* Keep variable names short for variables that are local to the function.
* Do not export a function or variable name outside the package until you
have an external consumer for it.
* Have setter or getter interfaces/methods to access/manipulate information in
Copy link
Contributor

Choose a reason for hiding this comment

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

I had to re-read this sentence four or five time before I understood what the recommendation was. Can we word this more clearly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed it :)

<import ceph-csi packages>

<import third-party packages>
```
Copy link
Contributor

@phlogistonjohn phlogistonjohn Apr 15, 2019

Choose a reason for hiding this comment

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

Is this really the convention here? I thought the common convention was:

  • stdlib
  • 3rdparty
  • local

If you look at the documenatation for "goimports" for example it says:

  -local string
        put imports beginning with this string after 3rd-party packages; comma-separated list

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, corrected it.

@humblec
Copy link
Collaborator Author

humblec commented Apr 24, 2019

@phlogistonjohn PTAL .. I have addressed the comments.

Copy link
Contributor

@ShyamsundarR ShyamsundarR left a comment

Choose a reason for hiding this comment

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

Do we need a contributor agreement in place or call out something around the same? As we are adding contributor documentation, thinking if we need to add the same with this PR or as a follow up.

* Keep variable names short for variables that are local to the function.
* Do not export a function or variable name outside the package until you
have an external consumer for it.
* Do not use named return values in function definitions. Use only the type.
Copy link
Contributor

Choose a reason for hiding this comment

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

@Madhu-1 and myself had a disagreement over this, I am fine with the rule being called out, but I would like to know why we should not use named returns and only use naked returns?
The answer of common style across code does not fit IMO as named returns are for short functions and not for all functions as per the language definition, hence all code cannot accommodate a single style.

README.md Outdated
@@ -20,6 +20,12 @@ CephFS CSI plugins, see documentation in `docs/`.

For example usage of RBD and CephFS CSI plugins, see examples in `examples/`.

## Contributing to this repo

Please follow [contibution guide](https://github.com/ceph/ceph-csi/tree/master/docs/coding.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not call the coding.md a contribution guide, I would call it the coding style guidelines, and hence reorder the links in this statement, with dev-guide first and then coding guidelines next.

* Do not panic() for errors that can be bubbled up back to user. Use panic()
only for fatal errors which shouldn't occur.
* Do not ignore errors using `_` variable unless you know what you're doing.
* Error strings should not start with a capital letter.
Copy link
Contributor

Choose a reason for hiding this comment

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

Linter catches this, as a result I would remove this from this list. Less rules to remember for the contributor.

* Do not ignore errors using `_` variable unless you know what you're doing.
* Error strings should not start with a capital letter.
* If error requires passing of extra information, you can define a new type
* Error types should end with `Error`.
Copy link
Contributor

Choose a reason for hiding this comment

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

The above 2 possibly need examples


### Logging

* If a function is only invoked as part of a transaction step, always use the
Copy link
Contributor

Choose a reason for hiding this comment

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

What is a transaction? Which ID would we be using here?

Copy link
Contributor

@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.

Most updates LGTM, but I do think you should address @ShyamsundarR's comments about the logging.

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Jun 3, 2019

@humblec anything pending on this one?

@humblec humblec force-pushed the contributing branch 6 times, most recently from 5aba12a to 529aeaf Compare June 4, 2019 05:57
Signed-off-by: Humble Chirammal <hchiramm@redhat.com>
@humblec
Copy link
Collaborator Author

humblec commented Jun 4, 2019

@Madhu-1 all the comments are addressed. PTAL

@humblec
Copy link
Collaborator Author

humblec commented Jun 4, 2019

Thanks @Madhu-1 , merging this.

@humblec humblec merged commit 1444231 into ceph:master Jun 4, 2019
@@ -1,7 +1,7 @@
# Ceph CSI 1.0.0

[Container Storage Interface
(CSI)](https://github.com/container-storage-interface/) driver, provisioner,
This repo containes [Container Storage Interface(CSI)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

containes/ contains

wilmardo pushed a commit to wilmardo/ceph-csi that referenced this pull request Jul 29, 2019
Madhu-1 pushed a commit to Madhu-1/ceph-csi that referenced this pull request Jun 20, 2024
Syncing latest changes from upstream devel for ceph-csi
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