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

nonspec: Add Pavel to maintainers #1271

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

marcelamelara
Copy link
Contributor

@marcelamelara marcelamelara commented Jan 7, 2025

Requesting to add @paveliak as a maintainer. He contributes as co-shepherd of the attested build environment track, so being able to manage issues and approve PRs in this track would be really helpful.

Relevant contributions to the track:

Signed-off-by: Marcela Melara <[email protected]>
Copy link

netlify bot commented Jan 7, 2025

Deploy Preview for slsa canceled.

Name Link
🔨 Latest commit c647684
🔍 Latest deploy log https://app.netlify.com/sites/slsa/deploys/677d5377f347f40008517d1f

@TomHennen
Copy link
Contributor

I think my only qualm, if I'm going to be especially pedantic, is that I can't find much public contribution from @paveliak to this project?

Am I

a) doing it wrong, you can see everything he's contributed HERE?
b) being too strict, Pavel has contributed in other spaces, and that's totally inline with maintainership?
c) other?

Copy link
Member

@mlieberman85 mlieberman85 left a comment

Choose a reason for hiding this comment

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

LGTM

@TomHennen
Copy link
Contributor

I think my only qualm, if I'm going to be especially pedantic, is that I can't find much public contribution from @paveliak to this project?

Am I

a) doing it wrong, you can see everything he's contributed HERE? b) being too strict, Pavel has contributed in other spaces, and that's totally inline with maintainership? c) other?

Seeing as how no one else is concerned, I'll retract my (minor) objection.

Welcome Pavel!

@lehors
Copy link
Member

lehors commented Jan 13, 2025

I actually feel a bit the same as Tom. It seems a bit odd to make someone who hasn't made a single commit a maintainer and doesn't show up in the list of contributors at all. This said, making a commit isn't a requirement. Our doc states that:

The criteria for becoming a Maintainer is documented in the SLSA Governance repository and copied here for convenience:

Demonstrated track record of PR reviews (both quality and quantity of reviews)
Demonstrated thought leadership in the project
Demonstrated shepherding of project work and contributors

The best way to get started is to regularly contribute and review pull requests.

So, as long as the above is true it is fine, but it might be worth making that claim explicit.
Note that I don't mean to be difficult, it's just a matter of fairness to other community members who may want to become maintainer.

@marcelamelara
Copy link
Contributor Author

The point about prior commits is fair, though I will say that my recommendation is based primarily on the third criterion:

Demonstrated shepherding of project work and contributors

We have, in the past, considered giving track shepherds maintainership, and with Triage permissions (which Pavel already has), he still hasn't been able to help me manage issues and PRs to the full extent needed to help the track progress. We also expect that @paveliak will be making actual commit contributions in the near future to this end. But I defer the final decision to the majority of the current maintainers.

@arewm
Copy link
Member

arewm commented Jan 14, 2025

For what it is worth, I chose to abstain from voting because I am not familiar with what his contributions have been.

@trishankatdatadog
Copy link
Member

Agree that lack of commits does not suggest that the Maintain role is appropriate. However, as @marcelamelara says, Pavel would need to assume the Triage role to manage issues, discussions, and PRs. Currently, we don't have a distinction between the two in our governance. Either we make room in the governance for a new group called SLSA Triagists (in line with name for role in medicine), or we clarify that Maintainers need not commit but only triage.

@marcelamelara
Copy link
Contributor Author

I appreciate the discussion. I do think that, if being a track shepherd isn't a sufficient contribution to become maintainer, we ought to emphasize the requirement for commits/merged PRs (maybe even set a min. expectation?) in our Contributing.md.

Either way, I'm not going to stand in the way of deferring this request if the majority of the other maintainers feel strongly about this.

@adityasaky
Copy link
Member

I thought I'd chime in with my reasoning for the approval from before.

I think it's worth noting that a) the SLSA maintainers decided the track is worth adding to the SLSA spec, and b) have already approved @paveliak as a shepherd for the track. I understand that shepherding is considered one way to demonstrate contributions to get maintainer bit (i.e., shepherding doesn't automatically mean you're a maintainer), but I want to highlight that Pavel's been reviewing PRs for this track (#1115, #1244) and, in the latter case, is even relied upon by maintainer(s) as a subject matter expert for changes to the track. Maybe this really means we need to (re-?)consider discussing maintainer bit on a track-by-track basis or so?

@lehors
Copy link
Member

lehors commented Jan 15, 2025

To clarify, I pointed out in my comment that making commits is NOT required and I didn't mean to imply that we should modify that.

My point was really meant to be that it's easy to figure out one's contribution when they have made commits and when it's not the case their contribution should be described as much as possible to back up the request. I think "He contributes as co-shepherd of the attested build environment track" is a bit weak.

But I'm not going to stand in the way of approving Pavel as a maintainer. Again my motivation is primarily to document our decision as well as possible to avoid raising any questions.

@marcelamelara
Copy link
Contributor Author

My point was really meant to be that it's easy to figure out one's contribution when they have made commits and when it's not the case their contribution should be described as much as possible to back up the request. I think "He contributes as co-shepherd of the attested build environment track" is a bit weak.

This is great feedback, thank you, @lehors ! And tbqh, I didn't expect to need to show ample evidence, when there have been various interactions with maintainers and community members on other PRs already. Anyway, I'm happy to add a list of key examples of contributions to be abundantly clear, if that'll help. And for future reference I still think it's reasonable to amend Contributing.md to ask for a few examples, if commits haven't been the primary contribution.

Copy link
Member

@lehors lehors left a comment

Choose a reason for hiding this comment

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

Thanks for the added info!

@lehors
Copy link
Member

lehors commented Jan 17, 2025

I count approval from 5 current maintainers to which we can add @marcelamelara. This gives us: 6 approve, 1 abstain, out of the 9 current maintainers. This meets the majority requirement.
I'm therefore merging this PR. Welcome @paveliak !

@lehors lehors merged commit 8941620 into slsa-framework:main Jan 17, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants