rustls / rcgen

Generate X.509 certificates
Other
321 stars 100 forks source link

Collaborative maintenance offer. #137

Closed cpu closed 1 year ago

cpu commented 1 year ago

Hi @est31,

This crate is a valuable part of the Rust X.509/TLS ecosystem and I'm very grateful for the work you've put in. I'm writing this issue to float the idea of collaborative maintenance of rcgen, potentially under the rustls organization banner. Is this something you would be open to?

I think having a community supporting rcgen will help realize some untapped potential:

From the Rustls side we can bring a lot of Rust, X.509, TLS, and PKI expertise. We also have resources that can help fund sustained contributions to projects like this as part of a broader mission to replace memory unsafe code in the TLS ecosystem. I see rcgen as an important part of this mission and both @ctz and @djc are supportive of this idea.

Some immediate priorities that come to mind:

If you'd like to join the Rustls Discord server we can talk through any hesitations you might have.

Thanks for considering this!

est31 commented 1 year ago

Thanks @cpu for reaching out.

First, I've seen your PR #136 and I really like the idea, I just didn't have the time yet to review it in detail. My plan is to do this in the weekend: My new job gives me less free time than before (I like it a lot though).

This brings us to your request. It is great to see that over the years (is it really already almost 5 years? Where is that time gone :o?), rcgen has evolved into something not just useful, but also used by many people in the ecosystem.

Given my personal situation giving me less time to work on non-work OSS projects now, I think it would definitely be great to have additional maintainers and this might be the best way to both grow rcgen, make it fit its user's needs better, and ensure future maintenance. I think the rustls group would be an ideal place for this, as it is a known and trusted brand in the rust ecosystem. It would be an honor for me to join the group.

Regarding your ideas, I have some input but those are details and can be discussed later, just wanted to give this quick feedback.

I have joined the discord and we can communicate there, but note that I have mostly left the place a year or so ago, so I'm not sure I want to be there on a long term basis.

djc commented 1 year ago

Great to hear, looking forward to move rcgen forward!

@est31 I propose we make you a member of the rustls org -- but we wouldn't add you to the committers team that would give you maintenance privileges for the other repositories for now. You should then be able to transfer the repository. I propose we give the rustls committers administrative privileges to the rcgen repo once it lives in the org, and it would be great if you can give github:rustls:publishers ownership on crates.io (as documented here).

How would you like to handle review? For simple reviews, we often get PRs merged within a day after submitting, but if you'd prefer to have more time to submit reviews before merging we could set up some window (say, 48 hours within a work week, or even make sure we always let a weekend pass -- I don't know when you typically work on rcgen) between submitting a PR and when we'd merge it at the earliest. (We're also happy to follow up on post-merge reviews, of course.)

cpu commented 1 year ago

First, I've seen your PR https://github.com/est31/rcgen/pull/136 and I really like the idea, I just didn't have the time yet to review it in detail. My plan is to do this in the weekend: My new job gives me less free time than before (I like it a lot though).

No rush on that, thanks for taking a look :-)

Given my personal situation giving me less time to work on non-work OSS projects now, I think it would definitely be great to have additional maintainers and this might be the best way to both grow rcgen, make it fit its user's needs better, and ensure future maintenance.

I'm really happy to hear you think so. I think it will be a good path forward for everyone.

Regarding your ideas, I have some input but those are details and can be discussed later, just wanted to give this quick feedback.

Sounds good. I propose we try to land #136 as-is, work on the administrative details @djc mentioned, and then tackle some of these ideas one by one with time for discussion/review as appropriate.

I have joined the discord and we can communicate there, but note that I have mostly left the place a year or so ago, so I'm not sure I want to be there on a long term basis.

That's fair, I know Discord isn't everyone's favourite platform. FWIW the volume of activity is on the lower side. In either case would you prefer we iron out details in GitHub, or an email thread, or somewhere else? I think we can be flexible if you have any preferences here.

djc commented 1 year ago

You should have an invitation to the GitHub rustls org.

est31 commented 1 year ago

Okay some updates:

So, this marks a new era for rustls then.

est31 commented 1 year ago

Regarding the PR merging policy, I'd propose the following:

Does this sound good?

djc commented 1 year ago

Regarding the PR merging policy, I'd propose the following:

  • I prefer a clean history, so no merge commits please.

  • If a PR has a clean history of its own and the commits would have been fine as components of their own, then it's fine to do a rebase merge, but otherwise, I prefer a squashed merge.

This is more or less what we do for the other crates -- we generally always make a clean history and then rebase merge that.

  • Generally, for the start, I'd like rustls maintainers to not merge PRs that have not been approved by me. I will look for PRs approved by the rustls maintainer team and will merge them after I checked them as well for issues. With time, I will give them less and less of a look and just press the approval/merge button. Eventually, I'd like this rule to just require anyone from the rustls maintainer team to approve a PR (who is not the author). This change would require written consent by me.

Makes sense to defer others being allowed to merge without your explicit approval, let's discuss that in the future.

  • If I have been pinged at least two times on a PR and at least one month has passed since the second ping without me giving my opinion on something, you can merge without my approval (still needing an approval of a rustls maintainer of course).

A month is pretty long, which could slow things down a lot... I would hope that your response times are generally faster than that.

  • Exceptions that don't need my approval: CVE emergency fixes. Also: small 1-10 line grammar fixes. Latter can also be merged/approved by the person filing the PR if they are rustls maintainer.

👍

  • As the creator, I reserve the right to directly push to master.

This one I'm not a fan of. Generally as a single maintainer adding contributors to my repositories I've always:

  1. Added branch protection to require that PRs are used for all changes, which has the benefit that (a) changes become much more visible (repository watchers get a notification) and that (b) CI has a chance to run before the change hits the master/main branch.
  2. Made it so that even PRs from me, as the prior sole maintainer, required PR approval from the other maintainer. IMO the main benefit of getting extra benefits is that all changes get a round of review. Given the fact that there are 2 full-time employed maintainers in the org, feedback should generally be quick.

IMO we should at least do (1) and ideally make sure (2) happens soon if not immediately. What are your concerns with these? Why do you want to retain the right to directly push to master?

est31 commented 1 year ago

Why do you want to retain the right to directly push to master?

It's faster to just push fixes/improvements without having to wait for others, creating branches, or waiting for CI. Of course, it might sometimes cause issues with CI being broken or such (as seen currently cc #141). But overall, I think it's still a good development model, as long as the number of people doing this is small (and the number of contributors in general).

But I can try out filing some PRs and seeing how well the reviews go.

cpu commented 1 year ago

For what it's worth my opinions match up with @djc here.

But I can try out filing some PRs and seeing how well the reviews go.

That would be great. I know we can turn around reviews on your work very quickly (within a day on average) and I'm not likely to provide any blocking feedback, just suggestions to consider.

cpu commented 1 year ago

(btw: can I ask what timezone you're in @est31? It's helpful when deciding when to handle bits of feedback, etc)

est31 commented 1 year ago

can I ask what timezone you're in

Physically I am in the Rome/Berlin timezone, but I am active all sorts of times really.

djc commented 1 year ago

Why do you want to retain the right to directly push to master?

It's faster to just push fixes/improvements without having to wait for others, creating branches, or waiting for CI. Of course, it might sometimes cause issues with CI being broken or such (as seen currently cc #141). But overall, I think it's still a good development model, as long as the number of people doing this is small (and the number of contributors in general).

But I can try out filing some PRs and seeing how well the reviews go.

Sounds good. Would it be okay to set up branch protection? If so, do you want to that or is it okay if I do it? I will allow admin override anyway, so it shouldn't really affect you. (I've found this can help prevent accidental pushes to master.)

How do you feel about renaming master to main, to match our other repos (and the current GitHub default)? I missed #143.

cpu commented 1 year ago

Would it be okay to set up branch protection? If so, do you want to that or is it okay if I do it? I will allow admin override anyway, so it shouldn't really affect you. (I've found this can help prevent accidental pushes to master.)

@est31 Thoughts on the above?

est31 commented 1 year ago

I don't seem to have admin access any more for some reason. If that is restored, we could set up branch protection. Personally I don't feel much value in it though, as generally people with push access should be trusted, and you can get in malicious changes via PRs, too.

cpu commented 1 year ago

I don't seem to have admin access any more for some reason.

I've corrected this in the repository settings.

est31 commented 1 year ago

Alright I have set up branch protection myself.

djc commented 1 year ago

Great! Let's close this issue for now and discuss further items in specific issues.