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

Collaborative maintenance offer. #137

Closed
cpu opened this issue Aug 17, 2023 · 17 comments
Closed

Collaborative maintenance offer. #137

cpu opened this issue Aug 17, 2023 · 17 comments

Comments

@cpu
Copy link
Member

cpu commented Aug 17, 2023

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:

  • Applying the best practices we use throughout the Rustls ecosystem (applying rustfmt, measuring test coverage, creating smaller modules, documenting a release process, etc).
  • Augmenting the parameter struct API model with an easier to understand builder model.
  • Implementing a command line tool atop the library ala minica to offer a paved road for consumers to make a quick standalone PKI without writing Rust code.
  • Helping tune generated certificates to have no findings/warnings with tools like Zlint.

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

Thanks for considering this!

@est31
Copy link
Member

est31 commented Aug 18, 2023

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
Copy link
Member

djc commented Aug 18, 2023

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
Copy link
Member Author

cpu commented Aug 18, 2023

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).

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
Copy link
Member

djc commented Aug 21, 2023

You should have an invitation to the GitHub rustls org.

@est31
Copy link
Member

est31 commented Aug 23, 2023

Okay some updates:

  • I've pushed some commits to split off the lib.rs file, it's almost half of its original size now. This might not be the end state, maybe I should file an issue.
  • I've accepted the invite to join the rustls org and transferred the repo to rustls. I understand that this doesn't automatically confer maintainer status to the other repos in the org.

So, this marks a new era for rustls then.

@est31
Copy link
Member

est31 commented Aug 23, 2023

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.
  • 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.
  • 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).
  • 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.

Does this sound good?

@djc
Copy link
Member

djc commented Aug 23, 2023

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
Copy link
Member

est31 commented Aug 23, 2023

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
Copy link
Member Author

cpu commented Aug 24, 2023

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
Copy link
Member Author

cpu commented Aug 24, 2023

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

@est31
Copy link
Member

est31 commented Aug 25, 2023

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
Copy link
Member

djc commented Aug 25, 2023

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
Copy link
Member Author

cpu commented Aug 29, 2023

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
Copy link
Member

est31 commented Aug 31, 2023

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
Copy link
Member Author

cpu commented Aug 31, 2023

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

I've corrected this in the repository settings.

@est31
Copy link
Member

est31 commented Aug 31, 2023

Alright I have set up branch protection myself.

@djc
Copy link
Member

djc commented Sep 1, 2023

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

@djc djc closed this as completed Sep 1, 2023
@djc djc mentioned this issue Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants