Skip to content

Document use of the maintainers field#12270

Merged
adamjstewart merged 3 commits intospack:developfrom
adamjstewart:docs/maintainers
Nov 15, 2019
Merged

Document use of the maintainers field#12270
adamjstewart merged 3 commits intospack:developfrom
adamjstewart:docs/maintainers

Conversation

@adamjstewart
Copy link
Copy Markdown
Member

No description provided.

@adamjstewart adamjstewart added the documentation Improvements or additions to documentation label Aug 4, 2019
@adamjstewart adamjstewart requested a review from tgamblin August 4, 2019 03:40
for their own software, as well as users who rely on a piece of
software and want to ensure that the package doesn't break. It
also gives users a list of people to contact for help when someone
reports a build error with the package.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is great.

Copy link
Copy Markdown
Member

@becker33 becker33 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for documenting this!

@adamjstewart
Copy link
Copy Markdown
Member Author

Ping @tgamblin

@adamjstewart
Copy link
Copy Markdown
Member Author

Ping ping

@tgamblin
Copy link
Copy Markdown
Member

I kind of want to get the action working before merging this.

@adamjstewart adamjstewart mentioned this pull request Nov 7, 2019
@adamjstewart
Copy link
Copy Markdown
Member Author

@tgamblin I'm going to refactor this PR (now that the docs have been split) and remove the word "automatically" for now. That way people can still know to add maintainers that can be manually notified.

Comment on lines +191 to +200
#. Add a comma-separated list of maintainers.

The ``maintainers`` field is a list of GitHub accounts of people
who want to be notified any time the package is modified. When a
pull request is submitted that updates the package, these people
will be requested to review the PR. This is useful for developers
who maintain a Spack package for their own software, as well as
users who rely on a piece of software and want to ensure that the
package doesn't break. It also gives users a list of people to
contact for help when someone reports a build error with the package.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is a great explanation of the multiple reasons why someone would want to be notified of any change to a recipe, and have that codified. However, I can't help wondering if this might be better as two directives: maintainers and watchers. I can certainly imagine that someone interested only in keeping track of changes that might break their software stack might not want themselves considered "maintainers" to the point of being a contact point for help in case of breakage. The separately-named directives would be treated the same way from the point of view of the review machinery, but would not be considered a point of contact for support. Thoughts?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@alalazo I don't think "users who rely on a piece of software and want to ensure the package doesn't break," are necessarily people who want to be on a, "...list of people to contact for help when someone reports a build error with the package." While both groups of people should be notified and/or hit up for reviews when the PR comes in, I don't think they should be conflated to that degree.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Personally I don’t think the increased complexity of having two lists is worth it. Conda-forge also uses a single maintainers field. The people who rely on a piece of software and want to be notified of a change are also likely to have encountered the same build problem and know how to fix it

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In my opinion maintainers will be a short and moderately stable group of people that commit some time to keep a recipe in a good state and can be contacted for review if that recipe is touched. This is directly related to the state of maintenance of the recipe and thus it seems right to have them listed in package.py.

People that want to just watch a package might be a lot more and a lot less stable - this is to say that having those people subscribed to / unsubscribed from notifications via issuing a PR doesn't feel right. Just my 2 cents.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@adamjstewart @alalazo That's perfectly fine, just not what I got from the wording.

@adamjstewart adamjstewart merged commit 5510bba into spack:develop Nov 15, 2019
@adamjstewart adamjstewart deleted the docs/maintainers branch November 15, 2019 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants