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

chore: add additional todos in the "change checklist" #3180

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ramfox
Copy link
Contributor

@ramfox ramfox commented Feb 11, 2025

Description

In order to be better prepared for releases, we are trying to "front load" more work on understanding the effects of breaking changes in our downstream dependencies. Part of this is to remind folks to take a look at the downstream dependencies when they make a breaking change, and file an issue (or they have capacity), open a PR on how to deal with those breaking changes in that dependency.

The PR template looks like this now:

Description

Breaking Changes

Notes & open questions

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.
    • List all breaking changes in the above "Breaking Changes" section.
    • Open an issue or PR on any number0 repos that are affected by this breaking change. Give guidance on how the updates should be handled or do the actual updates themselves. The major ones are:
      • quic-rpc
      • iroh-gossip
      • iroh-blobs
      • dumbpipe
      • sendme

Copy link

github-actions bot commented Feb 11, 2025

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3180/docs/iroh/

Last updated: 2025-02-17T16:30:32Z

Copy link

github-actions bot commented Feb 11, 2025

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: 93036cd

@@ -11,8 +11,14 @@
<!-- Any notes, remarks or open questions you have to make about the PR. -->

## Change checklist

<!-- Remove any that are not relevant. -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, you're daring to add explicit instructions! I had given up on this :D

@divagant-martian
Copy link
Contributor

I'm not following what

Open an issue or PR on any affected downstream dependency repos

means here. This should be clear even for newcomers and first time contributors.

Also, is this really a subtask of "document all breaking changes?

@ramfox
Copy link
Contributor Author

ramfox commented Feb 12, 2025

Also, is this really a subtask of "document all breaking changes?

I think yes! It's literally about documenting breaking changes, including adding instructions about how breaking changes are going to effect our other repos.

@divagant-martian
Copy link
Contributor

I tried to think how this work and now I'm a bit less convinced, so sorry for holding this up. Maybe at the end you are right anyway.

What's the case for a release that has multiple breaking changes? is there one issue per repo per breaking change then? What should these issues contain as content? I would think they have the "how to update" but it's that not what simply documenting the breaking changes here should achieve? I know these dependants are those we own but I would think our breaking changes description should be good enough to allow any dependant to update correctly

@matheus23
Copy link
Member

matheus23 commented Feb 13, 2025

I know these dependants are those we own but I would think our breaking changes description should be good enough to allow any dependant to update correctly

I think this is a good point, and it's true. That said, I think there's something else that should be documented in the issues that are opened in the downstream repositories.

Going back to one of the original issues that motivated this change (as far as I know): The change in #3110

The PR is a little short on breaking changes (i.e. it doesn't mention going from re-exporting quinn::Connection to our own Connection struct), but in the end this is not what was the problem.
The problem was the implications for e.g. iroh-blobs, where we used to assume that you can provide it with either a quinn::Connection from quinn, or from iroh. Even if the breaking changes were listed fully, this upgrade would still be harder for iroh-blobs, so ideally should've gotten more attention ahead of release day.

So the point of creating the issue is not to mention the breaking change in another place - but to take the architecture of the repository that needs to be updated into account and come up with an upgrade plan - or even propose a concrete upgrade with a PR.

@flub
Copy link
Contributor

flub commented Feb 13, 2025

So the point of creating the issue is not to mention the breaking change in another place - but to take the architecture of the repository that needs to be updated into account and come up with an upgrade plan - or even propose a concrete upgrade with a PR.

Yeah, I interpret this as: create a draft PR with a patch to the branch with your breaking change. If it's simple it's fine. If all things break you'll notice. At this point you can either try and solve it or describe what needs to be done in an issue I guess. But at least it'll be more visible.

As part of this I'm also guessing that the main branch of those repos will often end up living with a patch pointing to the iroh main branch in between releases.

But I'm making a lot more assumptions then is being described. I understand the template tries to strike a balance with keeping it light though.

@ramfox
Copy link
Contributor Author

ramfox commented Feb 17, 2025

Yeah, I interpret this as: create a draft PR with a patch to the branch with your breaking change. If it's simple it's fine. If all things break you'll notice. At this point you can either try and solve it or describe what needs to be done in an issue I guess. But at least it'll be more visible.

This is my intension.

@ramfox ramfox requested review from Frando, rklaehn and Arqu February 17, 2025 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

5 participants