Docs wip dachary contributor privilege #2972

ghost · 2018-02-05T09:36:01Z

Status

Ready for review

Description of Changes

https://forum.securedrop.club/t/adding-mickael-as-new-maintainer/375

was the first public discussion about elevating privileges for a
SecureDrop contributor. Since we're a small community there probably
is no need to estabish a complicated formal process. However, for the
sake of transparency we have a three step process that allows everyone
to discuss and possibly disagree with the privileges asked by a
contributor.

The contributor is to initiate the process because it would not make
sense to grant privileges to someone who is unlikely to use them. For
instance if a contributor has time to propose occasional pull
requests but is not motivated by reviewing.

Testing

read the documentation and point to my borken English ;-)

Deployment

N/A

Checklist

If you made changes to documentation:

Doc linting passed locally

conorsch · 2018-02-05T16:14:47Z

Great job on this, @dachary! By the way, I closed the associated forum thread you linked to.

Since these changes are docs-related, I'm going to build locally to make sure the formatting renders well (since reading the diff isn't a good indication of how the docs will be perceived). Will report back in-line with any follow-ups.

redshiftzero

thanks for preparing this :-)

redshiftzero · 2018-02-05T16:26:39Z

docs/development/contributor_guidelines.rst

+    * explains what is expected from the contributor before they can be granted the privilege
+* Step 3: the thread is closed
+
+The privileges of a developer who has not been active for a year or


what about 6 months? I feel like a year is a really long time (6 months is also what tor uses)

absolutely ok, nice to have a point of reference like tor as well 👍

redshiftzero · 2018-02-05T16:27:04Z

docs/development/contributor_guidelines.rst

+* Step 1: the contributor posts a message `in the forum
+  <https://forum.securedrop.club/>`__ asking for privileges (review or
+  merge, etc.).
+* Step 2: after at least a week, someone with permissions to grant such


how about "after at least a week, if there are no objections from current maintainers"

https://forum.securedrop.club/t/adding-mickael-as-new-maintainer/375 was the first public discussion about elevating privileges for a SecureDrop contributor. Since we're a small community there probably is no need to estabish a complicated formal process. However, for the sake of transparency we have a three step process that allows everyone to discuss and possibly disagree with the privileges asked by a contributor. The contributor is to initiate the process because it would not make sense to grant privileges to someone who is unlikely to use them. For instance if a contributor has time to propose occasional pull requests but is not motivated by reviewing.

There are a number of cases where a contributor with escalated privileges (for i18n or development) can be inactive for a few months. It does not necessarily mean they are no longer motivated or that they lost contact with SecureDrop for so long that they would not be comfortable using their privileges. However, if six months passed without activity we can safely assume it would not bother them to re-apply again. And establishing this revokation period is a non-controversial way to ensure all persons with privileges are currently active. This is also the period currently used by the tor project in a similar case.

ghost · 2018-02-05T17:38:22Z

thanks for the quick review :-) Amended and repushed !

redshiftzero · 2018-02-05T18:55:50Z

Looks good! just a linting failure:

/src/docs/development/contributor_guidelines.rst:142:Unexpected indentation.

conorsch

Sounds great! Requesting changes to address a few formatting nits.

@redshiftzero points out some linting violations, so I'm happy to address those too. Will post a separate branch with commits amending the formatting as described, suitable for cherry-picking if @dachary agrees.

conorsch · 2018-02-05T19:11:24Z

docs/development/contributor_guidelines.rst

+.. note:: the translation workflow is different from the code and
+          documentation development workflow, see the associated
+          :ref:`privilege escalation <i18n-administrator-permissions>`
+          documentation.


This section (and the corresponding Note over in the i18n docs) could be worded more clearly—I'll add some suggestions.

conorsch · 2018-02-05T19:12:52Z

docs/development/contributor_guidelines.rst

+      maintainers and adds a message to the thread
+    * explains what is expected from the contributor before they can
+      be granted the privilege
+* Step 3: the thread is closed


I have some formatting nits about the layout of steps 1-3; again, will add specific suggestions for inclusion shortly. The content is definitely solid, so my changes will be cosmetic.

conorsch · 2018-02-05T19:18:38Z

docs/development/contributor_guidelines.rst

+* Step 3: the thread is closed
+
+The privileges of a developer who has not been active for six months or
+more are revoked. They can apply again at any time.


👍, particularly with @redshiftzero's one year -> six months revision. It's worth pointing out that the definition of "active" will vary somewhat based on the specific privileges, e.g. those with "review" privilege must review PRs in order to remain active—but I won't clutter up the docs with such minutiae.

It is tempting to go into details indeed :-) I suspect we'll actually scrub privileges over longer periods when someone feels like it and the method will vary over time.

conorsch · 2018-02-05T19:19:14Z

docs/development/i18n.rst

+.. note:: the translation workflow is different from the code and
+          documentation development workflow, see the associated
+          :ref:`privilege escalation <contributor-permissions>`
+          documentation.


See above: any edits to the Note on the developer maintainer status should be matched by changes here.

Changes are strictly cosmetic: using an ordered list, rather than adding numbers inside an unordered list, and carefully matching capitalizing to make the documentation read more like a paragraph, even though it's broken up a bit. In the process, also resolving a linting error that was blocking merge.

We use separate processes for determining "maintainer" status for contributors to the technical codebase (e.g. application logic, tests, configuration/deployment) and contributors to the translations. Fortunately we have clear docs on both already: this change merely amends the language cross-linking the relevant parts of the docs.

conorsch · 2018-02-05T20:00:31Z

@dachary See two additional suggested edits here: https://github.com/freedomofpress/securedrop/tree/docs-wip-dachary-contributor-privilege-review Feel free to cherry-pick, or I can push up directly. Note that the first commit there addresses the linting failure present here, as well.

ghost · 2018-02-05T20:18:48Z

@conorsch @redshiftzero thanks again, you make it more than easy :-) Cherry-picked your commits and pushed them (no force push, nice).

conorsch · 2018-02-05T20:28:53Z

Looks great, @dachary! Just waiting for short-CI, then let's get it in.

codecov-io · 2018-02-05T20:34:59Z

Codecov Report

Merging #2972 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #2972   +/-   ##
========================================
  Coverage    89.28%   89.28%           
========================================
  Files           31       31           
  Lines         1810     1810           
  Branches       209      209           
========================================
  Hits          1616     1616           
  Misses         144      144           
  Partials        50       50

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4b34fe...35a50e5. Read the comment docs.

Conflicts: .circleci/config.yml Favored develop since admin test jobs were added in develop in #2758. install_files/ansible-base/roles/ossec/files/test_admin_key.pub install_files/ansible-base/roles/ossec/files/test_admin_key.sec Favored develop for these changes since these keys in 0.5.2 were erroneously both public keys (fixed in #2925). install_files/ansible-base/securedrop-configure.yml Deleted this file as it was removed in develop during the sdconfig refactor (#2758) from Ansible to Python. The locale prompt additions added in SecureDrop 0.5.2 were added in #2758 on develop. molecule/aws/scripts/app-tests.sh Favored develop since the addition of RTL language testing was added in #2930. molecule/aws/side_effect.yml Favored release/0.5.2 as these changes were due to the addition of Tor apt repo testing in CI against release branches (#2941). securedrop/Dockerfile Favored develop since all these gettext commands being merged into one RUN command was done in #2822 and is still on develop. docs/development/contributor_guidelines.rst Favored develop since these contributor guidelines were added recently in #2972.

ghost added docs goals: improve developer workflow labels Feb 5, 2018

conorsch requested review from conorsch and redshiftzero February 5, 2018 16:13

redshiftzero suggested changes Feb 5, 2018

View reviewed changes

Loic Dachary added 3 commits February 5, 2018 18:37

docs: cross linking privilege for i18n and development

7b7aa37

ghost force-pushed the docs-wip-dachary-contributor-privilege branch from 2700c07 to ede36a3 Compare February 5, 2018 17:37

redshiftzero previously approved these changes Feb 5, 2018

View reviewed changes

conorsch suggested changes Feb 5, 2018

View reviewed changes

Conor Schaefer added 2 commits February 5, 2018 20:57

ghost dismissed redshiftzero’s stale review via 35a50e5 February 5, 2018 20:18

conorsch approved these changes Feb 5, 2018

View reviewed changes

redshiftzero merged commit 929264c into develop Feb 5, 2018

redshiftzero deleted the docs-wip-dachary-contributor-privilege branch February 5, 2018 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Docs wip dachary contributor privilege #2972

Docs wip dachary contributor privilege #2972

ghost commented Feb 5, 2018

conorsch commented Feb 5, 2018

redshiftzero left a comment

redshiftzero Feb 5, 2018

ghost Feb 5, 2018

ghost Feb 5, 2018

redshiftzero Feb 5, 2018

ghost Feb 5, 2018

ghost commented Feb 5, 2018

redshiftzero commented Feb 5, 2018

conorsch left a comment

conorsch Feb 5, 2018

conorsch Feb 5, 2018

conorsch Feb 5, 2018

ghost Feb 5, 2018

conorsch Feb 5, 2018

conorsch commented Feb 5, 2018

ghost commented Feb 5, 2018

conorsch commented Feb 5, 2018

codecov-io commented Feb 5, 2018 •

edited

Loading

Docs wip dachary contributor privilege #2972

Docs wip dachary contributor privilege #2972

Conversation

ghost commented Feb 5, 2018

Status

Description of Changes

Testing

Deployment

Checklist

If you made changes to documentation:

conorsch commented Feb 5, 2018

redshiftzero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Feb 5, 2018

redshiftzero commented Feb 5, 2018

conorsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conorsch commented Feb 5, 2018

ghost commented Feb 5, 2018

conorsch commented Feb 5, 2018

codecov-io commented Feb 5, 2018 • edited Loading

Codecov Report

codecov-io commented Feb 5, 2018 •

edited

Loading