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.

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

[Needs SC vote before merging] SC: update collection inclusion procedure #1256

Merged
merged 2 commits into from May 14, 2024

Conversation

Andersson007
Copy link
Contributor

@Andersson007 Andersson007 commented Apr 9, 2024

Issues we need to solve

  • There's an influx of new inclusion requests
  • I review them but i spend more time and energy to ping the committee to conduct the second review
  • This is not good and frustrating for the collection maintainers: they do work to make their content compliant
  • In short, the two reviews and two SC approvals scheme doesn't work and is not gonna work if there no other committed volunteer from SC who will review stuff on a regular basis (if someone wants, please raise your hand)

Solution

  • Change the process so that if another person from SC wants to conduct the second review they can go ahead otherwise one review and approval is imo enough.
  • I expect someone raises concerns about possible quality deterioration but the two reviews scheme just doesn't work - I regularly published and publish calls for help but there's no interest. We should trust each other, really. If someone has doubts and wants to re-check that, say, a collection has CI running regularly, they can do it.
  • We should not make people wait or we should stop including new stuff

Let's discuss the above (copied) in the related topic.

@Andersson007 Andersson007 requested a review from a team as a code owner April 9, 2024 06:29
@ansible-documentation-bot ansible-documentation-bot bot added the sc_approval This PR requires approval from the Ansible Community Steering Committee label Apr 9, 2024
@Andersson007 Andersson007 marked this pull request as draft April 9, 2024 06:30
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Do I understand correctly that the main changes are:

  1. Both reviewers must be SC members (before only one of them had to be);
  2. The second review can skip most parts of the checklist and concentrate on documentation and development conventions?

* MUST be reviewed and approved by at least two persons, where at least one person is a Steering Committee member.
* For a Non-Steering Committee review to be counted for inclusion, it MUST be checked and approved by *another* Steering Committee member.
* Reviewers must not be involved significantly in development of the collection. They must declare any potential conflict of interest (for example, being friends/relatives/coworkers of the maintainers/authors, being users of the collection, or having contributed to that collection recently or in the past).
* MUST be reviewed and approved as compliant with the requirements by at least two Steering Committee members.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is now stricter than the original requirements (one SC person + another community person, which could be SC but doesn't have to be). Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's practically stricter as now there must be two SC approvals and how we can approve w/o actually reviewing a collection even selectively?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Checking a review (and quickly looking at very few files in the collection) is IMO less work than doing a partial review of the collection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@felixfontein i believe it's the same or even less work for the SC member who's checking (who's looking at the code and docs primarily anyway) but there will be no need for the second full review by the third person - so it's actually a significant simplification

@Andersson007
Copy link
Contributor Author

Do I understand correctly that the main changes are:

  1. Both reviewers must be SC members (before only one of them had to be);
  2. The second review can skip most parts of the checklist and concentrate on documentation and development conventions?
  1. See [Needs SC vote before merging] SC: update collection inclusion procedure #1256 (comment)
  2. Yes

@Andersson007 Andersson007 marked this pull request as ready for review May 14, 2024 05:45
@Andersson007
Copy link
Contributor Author

The vote to merge this PR was positive, merging, thanks everyone!

@Andersson007 Andersson007 merged commit ba860d2 into ansible:devel May 14, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sc_approval This PR requires approval from the Ansible Community Steering Committee
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants