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

consensus/docs: draft internal-CHANGELOG.md #3279

Merged
merged 1 commit into from
Aug 25, 2021

Conversation

nfrisby
Copy link
Contributor

@nfrisby nfrisby commented Jul 28, 2021

This PR adds a single new file, which contains its own motivation.

I had promised this file to @erikd a while ago, and @rvl just asked me for it. Rodney's request nicely motivated it by explaining that wallet, db-sync, etc may be integrating our changes especially long after we merge our PRs. So when I merge a PR and send a helpful Slack message warning them of breaking changes and how to integrate them, it's very easy for them to forget those notes event exist before they actually have to deal with the integration challenges.

@nfrisby nfrisby added consensus issues related to ouroboros-consensus documentation Network Documentation related tasks labels Jul 28, 2021
@nfrisby nfrisby force-pushed the nfrisby/start-internal-CHANGELOG branch from eed7703 to 0acf7a0 Compare July 28, 2021 18:41
@nfrisby nfrisby force-pushed the nfrisby/start-internal-CHANGELOG branch 3 times, most recently from 826f32c to 6757210 Compare July 28, 2021 18:51
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

Thanks @nfrisby!

ouroboros-consensus/docs/internal-CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@nc6 nc6 left a comment

Choose a reason for hiding this comment

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

Think this is a great idea. It would be nice if we could include the PR number, but I appreciate that's particularly tricky!

ouroboros-consensus/docs/internal-CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jasagredo jasagredo left a comment

Choose a reason for hiding this comment

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

I am okey with the idea of the ChangeLog although I would suggest a couple of improvements:

  • Keep links to code/PRs as changes can span several PRs.
  • Allow appending information to an entry to further refine it. If something is fundamentally wrong, probably just strikethrough it and add a reference to some explanation below, instead of deleting it.

@nfrisby
Copy link
Contributor Author

nfrisby commented Jul 29, 2021

Thanks for the comments everyone.

I think the immutability would be nice, but I likely overstated how important it is.

I'll update the draft based on the following notes, prompted by your comments. Thanks!

  • Downstream users deal with commit hashes. They have at least: the latest hash they've already integrated as well as the new hash they're considering how to integrate. This enables Rodney's clever plan of git diff revA revB -- ouroboros-consensus/docs/internal-CHANGELOG.md. I'll include that recommended command in the doc itself. If the user does that, then those of us writing the document can't really cause too much trouble unless we do something silly like change every line (eg DOS -> WIN line endings).

  • Moreover, downstream users deal with commit hashes from our mainline branch. That's where to find our "releases".

  • As of some point in our development history, all mainline commits are PR merge commits that list the PR number (ie we only use bors and never fast-forwards etc). Thus, indirectly, downstream users are picking some prefix of all of our merged PRs. But to emphasize: they don't ultimately care about PR numbers; they just want to know what changed between two hashes. The PR numbers would only help them (or someone else) do archaeology to figure out more details about a particular change. The hope is that the entries in this file will be complete enough that they won't have to do that.

  • I want the PR that makes the change to also add an entry to this log file. If we forget, and have to add the entry in a later PR, so be it -- we're human. But the goal is for the PR that makes the change to also update the log file. That would yield the most natural results.

  • We could put PR numbers in the doc itself: that number will already be known before the commit is finalized. But PR numbers are generally not of interest to our downstream users. And can be recovered more directly via a git log command.

    $ git log --first-parent --oneline 6cef814ca^..999f25035
    999f25035 Merge #3261
    f026e71e8 Merge #3278
    681351c06 Merge #3270
    36cb0b970 Merge #3230
    6cef814ca Merge #3275
    
  • Unlike PR numbers, we cannot predict the merge commit hash (that'd be self-referential). The idea to rely on git blame lets us pretend that the correct hash is automatically present in the file itself, since git blame --first-parent or git blame will display the relevant PR merge commit hash right next to each log entry.

  • I proposed including the date just so there is something in the file itself that indicates the time spans between entries. As David mentioned, it's not clear exactly which date this is/should be. Ideally, it's the timestamp of when the PR was merged; but that's self-referential. We could make the last step before merging the PR be to update the timestamp in the log, but that'll be easy to forget. So I think a practical and least-surprising/most-honest semantics is that the date in the file is "some time near" the PR merge timestamp.

@nfrisby nfrisby force-pushed the nfrisby/start-internal-CHANGELOG branch 4 times, most recently from ed123c6 to f4f29f2 Compare July 29, 2021 18:22
@nfrisby
Copy link
Contributor Author

nfrisby commented Jul 29, 2021

I've updated the PR, and I think the introduction/overview is much clearer now. Please let me know whether it addresses all your discussion points.

@nfrisby nfrisby force-pushed the nfrisby/start-internal-CHANGELOG branch from f4f29f2 to 11c2abe Compare July 29, 2021 18:45
@nfrisby nfrisby force-pushed the nfrisby/start-internal-CHANGELOG branch from 11c2abe to b01688c Compare August 2, 2021 14:58
@nfrisby
Copy link
Contributor Author

nfrisby commented Aug 2, 2021

I think I've now responded to everyone's comments. Please have a look again if you recall unfinished business :)

Copy link
Contributor

@jasagredo jasagredo left a comment

Choose a reason for hiding this comment

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

Looks good to me

ouroboros-consensus/docs/interface-CHANGELOG.md Outdated Show resolved Hide resolved
@nfrisby nfrisby force-pushed the nfrisby/start-internal-CHANGELOG branch from b01688c to d8acf55 Compare August 24, 2021 22:59
@nfrisby
Copy link
Contributor Author

nfrisby commented Aug 24, 2021

Hi @DavidEichmann. I think we're good to go here. Would you dismiss your Request Changes review? And then invoke bors r+? Thanks!

@DavidEichmann DavidEichmann self-requested a review August 25, 2021 09:00
@DavidEichmann
Copy link
Contributor

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 25, 2021

@iohk-bors iohk-bors bot merged commit 558c776 into master Aug 25, 2021
@iohk-bors iohk-bors bot deleted the nfrisby/start-internal-CHANGELOG branch August 25, 2021 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus issues related to ouroboros-consensus documentation Network Documentation related tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants