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.

Sign up for GitHub

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

Jump to bottom

devel/ tree minor edits #25099

Merged
merged 1 commit into from
May 10, 2016
Merged

devel/ tree minor edits #25099

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
118 changes: 91 additions & 27 deletions docs/devel/coding-conventions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,65 +50,129 @@ Updated: 5/3/2016
## Code conventions

- Bash

- https://google-styleguide.googlecode.com/svn/trunk/shell.xml
- Ensure that build, release, test, and cluster-management scripts run on OS X

- Ensure that build, release, test, and cluster-management scripts run on
OS X
Copy link
Member

Choose a reason for hiding this comment

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

this is a weird linewrap, but it renders correctly - can we equally indent it and still render?

Copy link
Member Author

@mikebrow mikebrow May 4, 2016 •
edited

Choose a reason for hiding this comment

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

Depends on the parsing tool. Some tools delete the extra whitespace in front of line two and after the new line, but not all tools. The key is the line is a section or paragraph if you will, with a particular tab over. However if you have spaces but no new section identifier like - or * or 1 then you are at the mercy of the implementation. This is why on many manuals when you look at it in a shell set to the number of cols it was drafted with (new lined at), all is good, but then when you shrink it a few lines you get the end of the shorter line (tail) as the beginning of the next but then you have an extra new line at the end of the tail then the next line with the spaces in front.

Having no spaces at the front of the next line in the paragraph tells the parsing tools that this is a continuing section. Further, the number of spaces at the beginning does not always correspond to the tabbed section. Think tabs vs spaces. Getting the number of spaces just right is a hit or miss and won't work for all tools. Thus, better to not guess.

Copy link
Member

Choose a reason for hiding this comment

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

puke. How many different renderings do we expect to work? Anything more
that GitHub (GFM) ?

On Tue, May 3, 2016 at 5:13 PM, Mike Brown notifications@github.com wrote:

In docs/devel/coding-conventions.md
#25099 (comment)
:

 - https://google-styleguide.googlecode.com/svn/trunk/shell.xml
    • Ensure that build, release, test, and cluster-management scripts run on OS X
    • Ensure that build, release, test, and cluster-management scripts run on
      +OS X

Depends on the parsing tool. Some tools delete the extra whitespace in
front of line two and after the new line, but not all tools. The key is the
line is a section or paragraph if you will, with a particular tab over.
However if you have spaces but no new section identifier like - or * or 1
then you are at the mercy of the implementation. This is why on many
manuals when you look at it at the cols it was new lined at all is good but
then when you strink it a few lines you get the end of the shorter line
(tail) as the beginning of the next but then you have an extra new line at
the end of the tail then the next line with the spaces in front. Having no
spaces at the front of the next line in the paragraph tells the parsing
tools that this is a continuing section. Further the number of spaces at
the beginning does not always correspond to the tabbed section. Getting the
number of spaces just right is a hit or miss and won't work f or all t
ools.. Thus better to not not guess.

—
You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/25099/files/f1b0b315c817c80452752e5861f957583138bae0#r61977101

Copy link
Member Author

@mikebrow mikebrow May 4, 2016 •
edited

Choose a reason for hiding this comment

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

I presume the two important ones are the linux man(ual) which are the cli text manual documents (MD.. compiled versions of .md files) and man pages which is being used by GitHub. I see there has been an effort to compile the kubectl-*.md docs into a linux manual.

Copy link
Member Author

@mikebrow mikebrow May 4, 2016 •
edited

Choose a reason for hiding this comment

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

@thockin To see the line wrap issue type man docs/devel/coding-conventions.md and make your window smaller.


- Go

- Ensure your code passes the [presubmit checks](development.md#hooks)
- [Go Code Review Comments](https://github.com/golang/go/wiki/CodeReviewComments)

- [Go Code Review
Comments](https://github.com/golang/go/wiki/CodeReviewComments)
Copy link
Member

Choose a reason for hiding this comment

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

same here


- [Effective Go](https://golang.org/doc/effective_go.html)

- Comment your code.
- [Go's commenting conventions](http://blog.golang.org/godoc-documenting-go-code)
- If reviewers ask questions about why the code is the way it is, that's a sign that comments might be helpful.
- [Go's commenting
conventions](http://blog.golang.org/godoc-documenting-go-code)
Copy link
Member

@thockin thockin May 3, 2016 •
edited

Choose a reason for hiding this comment

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

same, I won't call it out more.

- If reviewers ask questions about why the code is the way it is, that's a
sign that comments might be helpful.


- Command-line flags should use dashes, not underscores


- Naming
- Please consider package name when selecting an interface name, and avoid redundancy.
- e.g.: `storage.Interface` is better than `storage.StorageInterface`.
- Do not use uppercase characters, underscores, or dashes in package names.
- Please consider package name when selecting an interface name, and avoid
redundancy.

- e.g.: `storage.Interface` is better than `storage.StorageInterface`.

- Do not use uppercase characters, underscores, or dashes in package
names.
- Please consider parent directory name when choosing a package name.
- so pkg/controllers/autoscaler/foo.go should say `package autoscaler` not `package autoscalercontroller`.
- Unless there's a good reason, the `package foo` line should match the name of the directory in which the .go file exists.
- Importers can use a different name if they need to disambiguate.
- Locks should be called `lock` and should never be embedded (always `lock sync.Mutex`). When multiple locks are present, give each lock a distinct name following Go conventions - `stateLock`, `mapLock` etc.
- API conventions
- [API changes](api_changes.md)
- [API conventions](api-conventions.md)

- so pkg/controllers/autoscaler/foo.go should say `package autoscaler`
not `package autoscalercontroller`.
- Unless there's a good reason, the `package foo` line should match
the name of the directory in which the .go file exists.
- Importers can use a different name if they need to disambiguate.

- Locks should be called `lock` and should never be embedded (always `lock
sync.Mutex`). When multiple locks are present, give each lock a distinct name
following Go conventions - `stateLock`, `mapLock` etc.

- [API changes](api_changes.md)

- [API conventions](api-conventions.md)

- [Kubectl conventions](kubectl-conventions.md)

- [Logging conventions](logging.md)

## Testing conventions

- All new packages and most new significant functionality must come with unit tests
- Table-driven tests are preferred for testing multiple scenarios/inputs; for example, see [TestNamespaceAuthorization](../../test/integration/auth_test.go)
- Significant features should come with integration (test/integration) and/or [end-to-end (test/e2e) tests](e2e-tests.md)
- All new packages and most new significant functionality must come with unit
tests

- Table-driven tests are preferred for testing multiple scenarios/inputs; for
example, see [TestNamespaceAuthorization](../../test/integration/auth_test.go)

- Significant features should come with integration (test/integration) and/or
[end-to-end (test/e2e) tests](e2e-tests.md)
- Including new kubectl commands and major features of existing commands
- Unit tests must pass on OS X and Windows platforms - if you use Linux specific features, your test case must either be skipped on windows or compiled out (skipped is better when running Linux specific commands, compiled out is required when your code does not compile on Windows).

- Unit tests must pass on OS X and Windows platforms - if you use Linux
specific features, your test case must either be skipped on windows or compiled
out (skipped is better when running Linux specific commands, compiled out is
required when your code does not compile on Windows).

- Avoid relying on Docker hub (e.g. pull from Docker hub). Use gcr.io instead.
- Avoid waiting for a short amount of time (or without waiting) and expect an asynchronous thing to happen (e.g. wait for 1 seconds and expect a Pod to be running). Wait and retry instead.

- Avoid waiting for a short amount of time (or without waiting) and expect an
asynchronous thing to happen (e.g. wait for 1 seconds and expect a Pod to be
running). Wait and retry instead.

- See the [testing guide](testing.md) for additional testing advice.

## Directory and file conventions

- Avoid package sprawl. Find an appropriate subdirectory for new packages. (See [#4851](http://issues.k8s.io/4851) for discussion.)
- Libraries with no more appropriate home belong in new package subdirectories of pkg/util
- Avoid general utility packages. Packages called "util" are suspect. Instead, derive a name that describes your desired function. For example, the utility functions dealing with waiting for operations are in the "wait" package and include functionality like Poll. So the full name is wait.Poll
- Avoid package sprawl. Find an appropriate subdirectory for new packages.
(See [#4851](http://issues.k8s.io/4851) for discussion.)
- Libraries with no more appropriate home belong in new package
subdirectories of pkg/util

- Avoid general utility packages. Packages called "util" are suspect. Instead,
derive a name that describes your desired function. For example, the utility
functions dealing with waiting for operations are in the "wait" package and
include functionality like Poll. So the full name is wait.Poll

- All filenames should be lowercase

- Go source files and directories use underscores, not dashes
- Package directories should generally avoid using separators as much as possible (when packages are multiple words, they usually should be in nested subdirectories).
- Package directories should generally avoid using separators as much as
possible (when packages are multiple words, they usually should be in nested
subdirectories).

- Document directories and filenames should use dashes rather than underscores
- Contrived examples that illustrate system features belong in /docs/user-guide or /docs/admin, depending on whether it is a feature primarily intended for users that deploy applications or cluster administrators, respectively. Actual application examples belong in /examples.
- Examples should also illustrate
[best practices for configuration and using the system](../user-guide/config-best-practices.md)

- Contrived examples that illustrate system features belong in
/docs/user-guide or /docs/admin, depending on whether it is a feature primarily
intended for users that deploy applications or cluster administrators,
respectively. Actual application examples belong in /examples.
- Examples should also illustrate [best practices for configuration and
using the system](../user-guide/config-best-practices.md)

- Third-party code
- Go code for normal third-party dependencies is managed using [Godeps](https://github.com/tools/godep)

- Go code for normal third-party dependencies is managed using
[Godeps](https://github.com/tools/godep)

- Other third-party code belongs in `/third_party`
- forked third party Go code goes in `/third_party/forked`
- forked _golang stdlib_ code goes in `/third_party/golang`

- Third-party code must include licenses

- This includes modified third-party code and excerpts, as well

## Coding advice

- Go

- [Go landmines](https://gist.github.com/lavalamp/4bd23295a9f32706a48f)


Expand Down
72 changes: 57 additions & 15 deletions docs/devel/collab.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,44 +34,86 @@ Documentation for other releases can be found at

# On Collaborative Development

Kubernetes is open source, but many of the people working on it do so as their day job. In order to avoid forcing people to be "at work" effectively 24/7, we want to establish some semi-formal protocols around development. Hopefully these rules make things go more smoothly. If you find that this is not the case, please complain loudly.
Kubernetes is open source, but many of the people working on it do so as their
day job. In order to avoid forcing people to be "at work" effectively 24/7, we
want to establish some semi-formal protocols around development. Hopefully these
rules make things go more smoothly. If you find that this is not the case,
please complain loudly.

## Patches welcome

First and foremost: as a potential contributor, your changes and ideas are welcome at any hour of the day or night, weekdays, weekends, and holidays. Please do not ever hesitate to ask a question or send a PR.
First and foremost: as a potential contributor, your changes and ideas are
welcome at any hour of the day or night, weekdays, weekends, and holidays.
Please do not ever hesitate to ask a question or send a PR.

## Code reviews

All changes must be code reviewed. For non-maintainers this is obvious, since you can't commit anyway. But even for maintainers, we want all changes to get at least one review, preferably (for non-trivial changes obligatorily) from someone who knows the areas the change touches. For non-trivial changes we may want two reviewers. The primary reviewer will make this decision and nominate a second reviewer, if needed. Except for trivial changes, PRs should not be committed until relevant parties (e.g. owners of the subsystem affected by the PR) have had a reasonable chance to look at PR in their local business hours.
All changes must be code reviewed. For non-maintainers this is obvious, since
you can't commit anyway. But even for maintainers, we want all changes to get at
least one review, preferably (for non-trivial changes obligatorily) from someone
who knows the areas the change touches. For non-trivial changes we may want two
reviewers. The primary reviewer will make this decision and nominate a second
reviewer, if needed. Except for trivial changes, PRs should not be committed
until relevant parties (e.g. owners of the subsystem affected by the PR) have
had a reasonable chance to look at PR in their local business hours.

Most PRs will find reviewers organically. If a maintainer intends to be the primary reviewer of a PR they should set themselves as the assignee on GitHub and say so in a reply to the PR. Only the primary reviewer of a change should actually do the merge, except in rare cases (e.g. they are unavailable in a reasonable timeframe).
Most PRs will find reviewers organically. If a maintainer intends to be the
primary reviewer of a PR they should set themselves as the assignee on GitHub
and say so in a reply to the PR. Only the primary reviewer of a change should
actually do the merge, except in rare cases (e.g. they are unavailable in a
reasonable timeframe).

If a PR has gone 2 work days without an owner emerging, please poke the PR thread and ask for a reviewer to be assigned.
If a PR has gone 2 work days without an owner emerging, please poke the PR
thread and ask for a reviewer to be assigned.

Except for rare cases, such as trivial changes (e.g. typos, comments) or emergencies (e.g. broken builds), maintainers should not merge their own changes.
Except for rare cases, such as trivial changes (e.g. typos, comments) or
emergencies (e.g. broken builds), maintainers should not merge their own
changes.

Expect reviewers to request that you avoid [common go style mistakes](https://github.com/golang/go/wiki/CodeReviewComments) in your PRs.
Expect reviewers to request that you avoid [common go style
mistakes](https://github.com/golang/go/wiki/CodeReviewComments) in your PRs.

## Assigned reviews

Maintainers can assign reviews to other maintainers, when appropriate. The assignee becomes the shepherd for that PR and is responsible for merging the PR once they are satisfied with it or else closing it. The assignee might request reviews from non-maintainers.
Maintainers can assign reviews to other maintainers, when appropriate. The
assignee becomes the shepherd for that PR and is responsible for merging the PR
once they are satisfied with it or else closing it. The assignee might request
reviews from non-maintainers.

## Merge hours

Maintainers will do merges of appropriately reviewed-and-approved changes during their local "business hours" (typically 7:00 am Monday to 5:00 pm (17:00h) Friday). PRs that arrive over the weekend or on holidays will only be merged if there is a very good reason for it and if the code review requirements have been met. Concretely this means that nobody should merge changes immediately before going to bed for the night.
Maintainers will do merges of appropriately reviewed-and-approved changes during
their local "business hours" (typically 7:00 am Monday to 5:00 pm (17:00h)
Friday). PRs that arrive over the weekend or on holidays will only be merged if
there is a very good reason for it and if the code review requirements have been
met. Concretely this means that nobody should merge changes immediately before
going to bed for the night.

There may be discussion an even approvals granted outside of the above hours, but merges will generally be deferred.
There may be discussion an even approvals granted outside of the above hours,
but merges will generally be deferred.

If a PR is considered complex or controversial, the merge of that PR should be delayed to give all interested parties in all timezones the opportunity to provide feedback. Concretely, this means that such PRs should be held for 24
hours before merging. Of course "complex" and "controversial" are left to the judgment of the people involved, but we trust that part of being a committer is the judgment required to evaluate such things honestly, and not be
motivated by your desire (or your cube-mate's desire) to get their code merged. Also see "Holds" below, any reviewer can issue a "hold" to indicate that the PR is in fact complicated or complex and deserves further review.
If a PR is considered complex or controversial, the merge of that PR should be
delayed to give all interested parties in all timezones the opportunity to
provide feedback. Concretely, this means that such PRs should be held for 24
hours before merging. Of course "complex" and "controversial" are left to the
judgment of the people involved, but we trust that part of being a committer is
the judgment required to evaluate such things honestly, and not be motivated by
your desire (or your cube-mate's desire) to get their code merged. Also see
"Holds" below, any reviewer can issue a "hold" to indicate that the PR is in
fact complicated or complex and deserves further review.

PRs that are incorrectly judged to be merge-able, may be reverted and subject to re-review, if subsequent reviewers believe that they in fact are controversial or complex.
PRs that are incorrectly judged to be merge-able, may be reverted and subject to
re-review, if subsequent reviewers believe that they in fact are controversial
or complex.


## Holds

Any maintainer or core contributor who wants to review a PR but does not have time immediately may put a hold on a PR simply by saying so on the PR discussion and offering an ETA measured in single-digit days at most. Any PR that has a hold shall not be merged until the person who requested the hold acks the review, withdraws their hold, or is overruled by a preponderance of maintainers.
Any maintainer or core contributor who wants to review a PR but does not have
time immediately may put a hold on a PR simply by saying so on the PR discussion
and offering an ETA measured in single-digit days at most. Any PR that has a
hold shall not be merged until the person who requested the hold acks the
review, withdraws their hold, or is overruled by a preponderance of maintainers.


<!-- BEGIN MUNGE: GENERATED_ANALYTICS -->
Expand Down