Updates README per DOC-1506 #53

psoloway · 2015-03-19T20:15:38Z

Remove any "what this affects" sections, except where particularly warranted.
Make sure that for each parameter, where applicable, there is a data type and a default value.
Make sure that for each parameter that's applicable, there is a note if the parameter is optional.
Make sure the links in the README work and are accurate.
Update the link in the Contributing section to point here: https://docs.puppetlabs.com/forge/contributing.html
General copyediting.

1. Remove any "what this affects" sections, except where particularly warranted. 2. Make sure that for each parameter, where applicable, there is a data type and a default value. 3. Make sure that for each parameter that's applicable, there is a note if the parameter is optional. 4. Make sure the links in the README work and are accurate. 5. Update the link in the Contributing section to point here: https://docs.puppetlabs.com/forge/contributing.html 6. General copyediting.

ferventcoder · 2015-03-23T18:54:11Z

README.md

@@ -1,4 +1,4 @@
-ACL (Access Control List)
+acl


You really should still define what acl is somewhere.

I know it's down in the TOC, but it may be better to happen as soon as possible.

The explanation of ACEs moved down to the top of the Usage section (line 51). The Description section is supposed to stay pretty high-level, focusing on the question "what does it do?". This info seemed more pertinent to "how do I use it?".

For those of us who know what an ACL is, it's not a worry. For those that don't, the first question is what is it?

And I don't know on the capital versus non-capital. It's a TLA so....

It may be fine since the first time it is used in a sentence in overview it is spelled out.

In this context "acl" (lower-case, as defined in our style guide) is the name of the module itself, which is distinct from "ACL" (upper-case), the technology the module supports. ACL the technology doesn't get directly referenced until that Overview section you pointed out, so that's where the definition fits best.

A bit confusing, I know, but hopefully the end user won't even need to notice the distinction.

ferventcoder · 2015-03-23T19:05:08Z

With all of the reordering going on, it is extremely difficult to see what has actually changed. I think it would be better to split this up into two commits - one with the actual changes (FIRST), and then the reordering of sections.

psoloway · 2015-03-23T21:31:11Z

Hi Rob,

I feel your pain, but is there any way you can work around the messiness?
I'll be more mindful of this in the future, and save major reordering for
the end whenever possible—but in this case it would really take a lot of
time to get everything back to where it was.

For what it's worth, I'm finding the Split view a bit easier to read than
the unified diff.

On Mon, Mar 23, 2015 at 12:05 PM, Rob Reynolds [email protected]
wrote:

With all of the reordering going on, it is extremely difficult to see what
has actually changed. I think it would be better to split this up into two
commits - one with the actual changes (FIRST), and then the reordering of
sections.

—
Reply to this email directly or view it on GitHub
#53 (comment)
.

ferventcoder · 2015-03-24T15:30:41Z

README.md


-The name of the ACL resource; will be used for `target` if `target` is not set.
+You cannot specify inherited ACEs in a manifest; you can only specify whether to allow upstream inheritance to flow into the managed ACL. If your modeled resources don't follow this order, Windows complains. The `acl` type **does not** enforce or complain about ACE order.


One thing here, the acl type will order them as they are put into the manifest, so it does enforce the order specified, it just doesn't complain about it being wrong. However you want to wordsmith that.

How about this:

You cannot specify inherited ACEs in a manifest; you can only specify whether to allow upstream inheritance to flow into the managed ACL. If your modeled resources don't follow this order, Windows complains. The acl type applies your resources in the order they appear in your manifest, and does not warn you if the order is incorrect.

Actually that helped a bit. I see now that we should spell out that the order is based on the paragraph above, and maybe change that to an ordered list.

How about this:

Each ACE has one of four possible statuses (in the order they are applied):

explicit deny

explicit allow

inherited deny

inherited allow

The order of ACEs within an ACL determines which one is applied first. If your modeled resources don't follow this order, Windows complains. The acl type applies the ACEs in the order they appear in your resource, but does not warn you if the order is incorrect.

NOTE: You cannot specify inherited ACEs in a manifest; you can only specify whether to allow upstream inheritance to flow into the managed ACL.

updated my comment - I missed that there is the Windows relevant ACL / ACE application first, then the acl resource and how it does its work.

I think with this change, everything is probably good for merge.

Excellent, thanks!

To anyone watching: I've got a quick call scheduled with @ferventcoder tomorrow morning to refine my understanding of this paragraph. I might take one more crack at clarifying it before updating the PR.

Rewords the Description and Usage intro based on comments. Also fixes a broken anchor link in the table of contents.

We refer to a message that Windows users might receive if applying ACEs in a non-standard order. Adding the specific text for the sake of searchability.

ferventcoder · 2015-03-25T19:33:28Z

I'm +1 to these changes

Updates README per DOC-1506

ferventcoder reviewed Mar 23, 2015
View reviewed changes

ferventcoder reviewed Mar 24, 2015
View reviewed changes

psoloway added 2 commits March 25, 2015 11:47

Adjusts updates per feedback

b364847

Rewords the Description and Usage intro based on comments. Also fixes a broken anchor link in the table of contents.

Adds text of possible warning message

2c24e95

We refer to a message that Windows users might receive if applying ACEs in a non-standard order. Adding the specific text for the sake of searchability.

ferventcoder added a commit that referenced this pull request Mar 25, 2015

Merge pull request #53 from psoloway/master

f3f9fd7

Updates README per DOC-1506

ferventcoder merged commit f3f9fd7 into puppetlabs:master Mar 25, 2015

LukasAud added the maintenance label Jun 22, 2023


		The name of the ACL resource; will be used for `target` if `target` is not set.
		You cannot specify inherited ACEs in a manifest; you can only specify whether to allow upstream inheritance to flow into the managed ACL. If your modeled resources don't follow this order, Windows complains. The `acl` type does not enforce or complain about ACE order.

Updates README per DOC-1506 #53

Updates README per DOC-1506 #53

Uh oh!

Conversation

psoloway commented Mar 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ferventcoder commented Mar 23, 2015

Uh oh!

psoloway commented Mar 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ferventcoder commented Mar 25, 2015

Uh oh!

Uh oh!