RFD 104: Discussion about rfd HOT 7 OPEN

It's a nit, and I really tried to let it slip by, but I have to say something one last time. Writing "Copyright (c)" is literally the same as typing "Copyright Copyright". The symbol is simply typographic shorthand for the word. Someone said that this convention came from Bryan but even his newer commits use "Copyright 2017 Joyent, Inc."

from rfd.

I took a pass through this. Unsurprisingly, most of my comments are minor, nitty things. It's also likely most of them are not related to anything you actually wrote or changed. I've split up my comments by the closest related section heading in the document.

General questions/comments

• Can we add a table of contents and hyperlinks to sections?

Section-specific questions/comments

Repository Naming

• Given that there are several rules for repository naming listed in this document in addition to language-specific guidelines for naming, I would reword "See language-specific guides for details" to "See language-specific guides for additional details" or "See language-specific guides for additional guidelines". In other words, I don't think the language used is always as relevant for the practice of repository naming as it for other practices discussed in this document.

Licenses and copyright

• nit: s/There should only be a single year/There should be a single year

Lint checks

• It seems like we might be missing an "s" in the last sentence (s/detail/details).

Code Comments

• Given that the example block comment in panic.c is a link to a link number for a file in master, I think we should ensure doesn't break in the future by either: pointing it to a specific version of the comment instead of the master branch (e.g., https://github.com/joyent/illumos-joyent/blob/403b9b2581c0e421d5fd8a74975df28290e276e5/usr/src/uts/common/os/panic.c#L30-L122 instead of https://github.com/joyent/illumos-joyent/blob/403b9b2581c0e421d5fd8a74975df28290e276e5/usr/src/uts/common/os/panic.c#L29) or just linking to the file on the master branch, since that file is highly unlikely to move or disappear.

Debuggability

• Since we mention that we use kang for debugging, should we also mention prometheus (and node-artedi)?
• There are a lot of assertions of what an engineer should do when writing a new component to improve post hoc debuggability, such as "it should be possible to understand excessive memory usage", without any concrete advice on how to do this. I think it would be great if we could either provide guidance on how to implement these suggestions here or point to existing documentation. For instance, we could point to language-specific docs from the engineering guide, then link to MDB and Node.js from the Node-specific guide.

Service (process) management with SMF

• There are a lot of man pages referenced in this section. To be consistent with the rest of the document, we should put their names in backticks: e.g., smf(5).

Miscellaneous Best Practices

• Comma splice in the first bullet point!
• This is really nitty, but it seems strange to me that "Macbook" is used to generally refer to our work laptops.

Security and production code deployment process

• The sentence “New changes affecting security are reviewed by a developer other than the person who wrote the new code” makes it sound as if we don't do always do this for non-security related code. I think we should take care to imply that! Instead, we could say: "New changes affecting security must be reviewed by a developer other than the person who wrote the new code” or "As with all changes, changes affecting security must be reviewed by a developer other than the person who wrote the new code” to communicate a sense of urgency about soliciting review without making it sound as it is ever acceptable not to do so.

from rfd.

This is really nitty, but it seems strange to me that "Macbook" is used to generally refer to our work laptops.

I concur, as I don't have one anymore! Perhaps "workstation"?

from rfd.

This looks really good.

I have a few comments/questions. Some of them may be out of the scope. If they're out of scope, I'd be happy to contribute documentation to wherever they should go.

• One thing that I would like is to somehow link to live examples of a good CONTRIBUTING.md and README.md.
• If all components must use a Bunyan logger, maybe we should have a note to see the programming language's best practices document for a link to the implementation of Bunyan (node-bunyan, libbunyan, GoBunyan, perlbunyan, etc.).
• At what point can we claim to be able to use a language in production? Is there a subset of things that must be true before its use is allowed? For example, we must have a Bunyan-compliant logging tool, binaries/executables must be able to run inside native zones, we must be able to examine core dumps in MDB, etc.. I'm curious if we should outline any of these things before we get too anxious about running new languages in production.
• Related to above, do we allow applications to run in KVM or LX zones if it is required for compatibility, and why/why not?
• Some libraries write logs (cueball, restify), and some libraries don't write logs (verror, vasync, node-artedi). I'm not sure if this is something we need to explain, but I thought I would point it out if others think it deserves explanation.
• Is it fine to lump a bunch of unrelated issues together into one commit? For example, can I have a single commit that contains both a bug fix for subsystem X, a feature for subsystem Y, and doc updates for subsystem Z?
• Will the engineering guide in eng.git or this document be the canonical copy that we link to from README/CONTRIBUTING documents?

Maybe some of these things are too prescriptive for this document. I'm not really sure, but I would be interested to hear thoughts from others. I'm guessing that we have opinions on these things, but they aren't written down anywhere.

Thanks! This looks good and is helpful.

from rfd.

Thanks everybody for taking a look! Below are responses to Jordan's and Josh's feedback, with changes applied in commit eb907b6.

Can we add a table of contents and hyperlinks to sections?

mo.joyent.com hosts a restdown-rendered version of the Engineering guide that includes a table of contents. We may want to improve this (e.g., adoc + auto-render-to-Manta). My bias is to separate that discussion, since it seems like it could be its own complex discussion. If it seems important here (e.g., because it also affects how we want to organize the content), that's fair.

Repository Naming

Given that there are several rules for repository naming listed in this document in addition to language-specific guidelines for naming, I would reword "See language-specific guides for details" to "See language-specific guides for additional details" or "See language-specific guides for additional guidelines". In other words, I don't think the language used is always as relevant for the practice of repository naming as it for other practices discussed in this document.

Updated.

Licenses and copyright

nit: s/There should only be a single year/There should be a single year

Updated.

It seems like we might be missing an "s" in the last sentence (s/detail/details).

Updated.

Given that the example block comment in panic.c is a link to a link number for a file in master, I think we should ensure doesn't break in the future by either: pointing it to a specific version of the comment instead of the master branch (e.g., https://github.com/joyent/illumos-joyent/blob/403b9b2581c0e421d5fd8a74975df28290e276e5/usr/src/uts/common/os/panic.c#L30-L122 instead of https://github.com/joyent/illumos-joyent/blob/403b9b2581c0e421d5fd8a74975df28290e276e5/usr/src/uts/common/os/panic.c#L29) or just linking to the file on the master branch, since that file is highly unlikely to move or disappear.

Updated.

Debuggability

Since we mention that we use kang for debugging, should we also mention prometheus (and node-artedi)?

I have reworked the Debuggability section somewhat and mentioned node-artedi. But see the next point.

I've also updated RFD 105 to mention node-artedi in a new Metrics section.

There are a lot of assertions of what an engineer should do when writing a new component to improve post hoc debuggability, such as "it should be possible to understand excessive memory usage", without any concrete advice on how to do this. I think it would be great if we could either provide guidance on how to implement these suggestions here or point to existing documentation. For instance, we could point to language-specific docs from the engineering guide, then link to MDB and Node.js from the Node-specific guide.

I have added a note to the Debuggability section that points to the language-specific docs.

For context: this content originally came from a draft of a Node.js-specific best practices guide, and we decided to separate the general principles from the language-specific recommendations so that we could crisply define what's required from guides for new languages. So this section is necessarily a little vague, but there's a corresponding section in the Node guide (RFD 105) that covers the specific suggestions.

Service (process) management with SMF

There are a lot of man pages referenced in this section. To be consistent with the rest of the document, we should put their names in backticks: e.g., smf(5).

Updated.

Miscellaneous Best Practices

Comma splice in the first bullet point!

I've rewritten this bullet point.

This is really nitty, but it seems strange to me that "Macbook" is used to generally refer to our work laptops.

I've reworded this to make clear that we're talking about any environment other than a SmartOS zone of the same image as the program you're working on.

Security and production code deployment process

The sentence “New changes affecting security are reviewed by a developer other than the person who wrote the new code” makes it sound as if we don't do always do this for non-security related code. I think we should take care to imply that! Instead, we could say: "New changes affecting security must be reviewed by a developer other than the person who wrote the new code” or "As with all changes, changes affecting security must be reviewed by a developer other than the person who wrote the new code” to communicate a sense of urgency about soliciting review without making it sound as it is ever acceptable not to do so.

I think these are good changes, particularly since the tense of this paragraph does not match the rest of the document. However, last time I changed this block, I was told it had been added for PCI compliance, and I do not know what the restrictions are about changing it. I will try to find out.

from rfd.

I've incorporated Kody's feedback below into commit 06618a1.

One thing that I would like is to somehow link to live examples of a good CONTRIBUTING.md and README.md.

I think we should probably just put a few different CONTRIBUTING.md files into the eng.git repository that should be copied directly into other repositories. Namely:

• CONTRIBUTING.md for Triton projects.
• CONTRIBUTING.md for Manta projects.
• CONTRIBUTING.md for standalone projects.

As for README: there's a bunch of different possible content here, so it may make more sense to link to examples. I'm tempted to point to node-verror or node-fast for standalone repos or manta-marlin for a Manta repo, but then, I would suggest those because I wrote them. I'd welcome other suggestions for good READMEs.

If all components must use a Bunyan logger, maybe we should have a note to see the programming language's best practices document for a link to the implementation of Bunyan (node-bunyan, libbunyan, GoBunyan, perlbunyan, etc.).

That's a good idea -- and that would go in the RFD for the corresponding language guide. RFD 105 (Node.js) has a link to node-bunyan.

At what point can we claim to be able to use a language in production? Is there a subset of things that must be true before its use is allowed? For example, we must have a Bunyan-compliant logging tool, binaries/executables must be able to run inside native zones, we must be able to examine core dumps in MDB, etc.. I'm curious if we should outline any of these things before we get too anxious about running new languages in production.

I would say that we must have a best practices guide that provides workable solutions for everything that the General Principles section says is language-specific, including the notes under Repository naming, Coding style, Lint checks, Automated testing, and Debuggability. I've added a note to the "Programming language" section about this.

Related to above, do we allow applications to run in KVM or LX zones if it is required for compatibility, and why/why not?

I've added a new section called "Deployment environment" with suggested guidelines. I expect there may be some discussion about it.

Some libraries write logs (cueball, restify), and some libraries don't write logs (verror, vasync, node-artedi). I'm not sure if this is something we need to explain, but I thought I would point it out if others think it deserves explanation.

Good call. I've added a note at the end of the Logging section about this. I'm not sure if that's prominent enough.

Is it fine to lump a bunch of unrelated issues together into one commit? For example, can I have a single commit that contains both a bug fix for subsystem X, a feature for subsystem Y, and doc updates for subsystem Z?

Yes. I've added a paragraph to the end of the "Commit messages" section.

Will the engineering guide in eng.git or this document be the canonical copy that we link to from README/CONTRIBUTING documents?

I don't propose changing the canonical location or anything. It was suggested that the content go into the RFD to make it easier to see, though I do wonder if we would be better off removing the content from the RFD once it becomes incorporated into the real guide so that people don't wind up looking at outdated versions here as the real guide evolves.

from rfd.

On https://github.com/joyent/rfd/blob/master/rfd/0104/README.md#commit-messages

While looking at my 3rd CR for the day for a small change where 90% of the CR discussion was about the format of the commit message, may I suggest:

• that we drop the option of trimming the issue title lines to 80 columns; and
• the addition of a pre-block showing the general structure before the given examples.

Rationale for dropping the 80 columns thing: Fine if we want an 80 columns limit for code, but I don't see value in having an option here for commit messages. It invites trimming out relevant context from the ticket title for what gain? If we really care about trying to keep these short, the burden should be on encouraging more succinct ticket titles. One anecdote related to attempting to balance between "the commit message ticket title should include relevant context" and "keeping to under 80 columns" is https://cr.joyent.us/#/c/2381/1..2//COMMIT_MSG

Rationale for the pre-block I've added (see patch below): At least for the way I read tech documents, skimming to see a general template for the structure helps me grok the surrounding prose.

Suggested patch: https://gist.github.com/trentm/3c96cbfcff02d57b1280c02101e49b7d

from rfd.