Please note that some commits may be excluded from this release if they’re not relevant, or not modifying the GuestOS image. Additionally, descriptions of some changes might have been slightly modified to fit the release notes format.
To see a full list of commits added since last release, compare the revisions on GitHub.
The two SHA256 sums printed above from a) the downloaded CDN image and b) the locally built image, must be identical, and must match the SHA256 from the payload of the NNS proposal.
previous executed release (described as a ‘removed commit’ in the proposal summary),
the ‘same change under a different commit’,
and the ‘merge base’ commit
… are all highlighted in blue above (and labelled to make it clearer).
I’ve verified that the ‘removed commit’ and ‘same change under a different commit’ are binary equivalent. This step (on the part of a reviewer) could be easily automated for merge requests (due to the clearly modelled relationship between a source and merge commit). Cherry picked commits have a much looser relationship (as is the case with the previous executed release/‘removed commit’).
If the previous executed release branch had been cut using a merge commit rather than a cherry picked commit the git graph would also have been much clearer, e.g. →
Merging the previous executed release back into main in the example above is an unnecessary step, but I think it’s visually clearer (indicating that there’s actually no divergence).
I’m coming at this from the stand point of a reviewer seeking as many ways as possible to make IC-OS releases easier and more convenient to verify and review (in the hope that more people will be more likely to get involved in the future).
Do you have any thoughts on these ideas @DRE-Team? My thinking is that this sort of approach avoids the need to explain any kind of divergence (as in this proposal summary).
Shouldn’t all commits that modify GuestOS be considered relevant and included in the list of changes? For many voters out there I expect this is the only list they look at. Why not let each person make their own judgement call on which GuestOS changes are relevant/important to them?
Similarly, the above commit is relevant because it modifies HTTP handler configuration (and is
included in the list of changes above). Commit dd0be35c also modifies HTTP handler configuration (reducing max_tracing_flamegraph_concurrent_requests from 10 to 5), but it’s not included in the list of changes above.
i’m looking into some way to share all the list of commits that modify guestos. for release notes, we’d still want to make it even less than that just so we don’t have commits that don’t contribute much. for this particular commit, it was auto excluded from release notes based on some filter. will have to debug why exactly and get back to you next week. in terms of the change itself, it is kinda important but not really. it’s whitelisting a private ipv6 prefix so it shouldn’t make a huge difference. similar to the line above. i could’ve probably optimized and replaced the previous line with fd00::/8
we wouldn’t have linear history on master in that case. it’s also not necessarily the case that we always want to do that. that being said, merging the RC branch into master would probably work.
in this case i just wanted to make sure there’s accurate description of what exactly is happening. if you have any other ideas, we could try implementing something else for future releases.
@Lorimer i think in both cases looks like this is the cause for exclusion:
i’ll try to improve this logic a bit. we also plan on improving package dependencies for guestos so we can be less aggressive on these heuristics that exclude commits from release notes,
Thanks @Luka! Looks like there’s a good chance that logic is the issue. I raised questions about that specific line a few weeks ago
(EXCLUDED_TEAMS has since been renamed to NON_REPLICA_TEAMS)
The master branch would describe the reality of the situation. Isn’t that better? There are already non-linearities in the deployment history. Can I ask what problems representing this on the master branch would cause? (at least in cases where there’s genuinely no divergence)
This sounds promising, except that release notes will intentionally (as opposed to accidentally) exclude commits that modify GuestOS. Why would DFINITY want to do this? In some ways this could be seen as worse. It certainly seems like a bad precedent to set for such critical proposals. I think the principle here is the most important thing (which is why I’ll reject this proposal, in peaceful protest of the obscured changes - I suspect there are more).
Thanks Luka. Do you have any thoughts about the deployment target commit message convention that we discussed with @satlast week in our CodeGov meeting. My impression was that this was being investigated.
this is a matter of trunk based development. sometimes there’s a hotfix on a previous release, but that hotfix is never merged into main. in this case it was. we can complicate the setup and sometimes merge and sometimes not. would this simplify review process? it seems if we complicate release process, review process is going to complicate too, or at least in this specific case.
we’d want the release notes to represent functional changes to replica instead of pure changelog. we still want to provide some methods to simplify code review. i don’t think either of these two approaches are exclusive of the other.
it’s just another heuristic in the end. i overall don’t have confidence that this would be better than relying on bazel to identify which commit changed what. we could use this in conjunction with bazel, but i don’t see how it could bring a benefit. the problem is that you’d have to choose one when there’s a conflict between the two. i would almost always trust bazel instead of a developer. it’s just much easier for developer to say that deployment target is “none” or some equivalent (we’d need this for example if we change tests or CI, there’s no deployment target). that’s not to say that developer is always going to be wrong, but it’s much easier for developer to be wrong than bazel to be “wrong”. i quote “wrong” because bazel technically cannot be wrong. if bazel says the commit changes some dependencies, we’ll get a different guestOS build.
i didn’t talk with @sat about this since he’s currently out on vacation.
If the change doesn’t belong on the master branch, it makes sense not to merge it. If it does, it can be merged. Aside from the git graph being more visually accurate, it means a community member can programmatically verify that merge commits don’t include changes other than what’s claiming to be merged (allowing them to inspect the cases where they do, e.g. due to merge conflicts). This would have saved needing to manually check for binary equivalence on this proposal.
If you’d like to keep things consistent (to reduce operational complexity), why not always ensure all deployed changes join the master branch. The scenario you described is what reverting is for. This makes the master branch a more comprehensive reflection of production over time.
Why open the door to obfuscated changes? DFINITY sets an example for other proposers to follow in the future. Why not display all changes that voters are voting on as a matter of principle?
That’s what I’m suggesting. If the developer and bazel disagree, then you can inspect and either fix another issue with the proposal summary tool, or remind the developer in question of the commit message convention. There are also already exiting conventions that committers are following consistently. I’m sure they could handle another (arguably more important) one.
Are you sure this is true? If the registry canister is modified (for example), does this alter the GuestOS build hash?
Please note that some of our reviewers commented that the errors in the summary from last week have been corrected. All reviewers adopted the proposal except @Lorimer, who provided this explanation for why he rejected…" Rejected due to obscured changes, which is something I think needs to be considered taboo." Further details about this decision can be found in prior posts he made in this thread on the forum.
At the time of this comment on the forum there are still 2 days left in the voting period, which means there is still plenty of time for others to review the proposal and vote independently.
We had several very good reviews of the Release Notes on these proposals by @Zane, @cyberowl, @ZackDS, @massimoalbarello, @ilbert, @hpeebles, and @Lorimer. The IC-OS Verification was also performed by @tiago89. I recommend folks take a look and see the excellent work that was performed on these reviews by the entire CodeGov team. Feel free to comment here or in the thread of each respective proposal in our community on OpenChat if you have any questions or suggestions about these reviews.
but then we’d merge a change to master just to revert it
the problem is that we cannot automate this. so we’re introducing operational complexity and introduce a chance for errors. how much are we getting in return and is it worth it?
i think the release notes are supposed to be just an overview of what was changed. adopted proposals end up on release page: https://dashboard.internetcomputer.org/releases
it makes sense that for review you’d want to get a list of all the things that changed in detail without any moderating. but after it’s adopted, i’d argue it’s much nicer that we end up with a sensible release notes instead of unmoderated mess on release page.
if you’d want to review the code, i think it’s best to not even go commit by commit, but instead just see the entire difference in code between last release and the current.
the issues we experience over the past few weeks have nothing to do with bazel part. bazel filters out commits that do not modify the replica. then we further remove some commits because we think they’re not relevant.
introducing the deploy target message would just make heuristic different. whether we’re trying to ask developers to guess if this is going to change replica, or we algorithmically try to exclude some package that do not make sense, it’s still just a guess. i think in fact the package filter works quite well since it’s very conservative in what it kicks out, and can be easily tuned. the issue we had with teams for this release is not a great heuristic IMO. i’ll see this week what we can do with it - either substantially improve it or remove it. developers tagging the deployment target i think would remove more commits than even these both together. i mean, i’m still just guessing, but i base it on amount of “chore” commits we have. i think we can expect the result would be quite similar if we had deployment target. i think lots of commits would be “deployment target: none”. if we want to ask developers to change their workflow to introduce more overhead, i think we need to have a reasonable assumption that we’re gonna get a good return there. what makes you convinced that we would?
almost 100%. it depends on the compiler in the end, but it’s very likely that if something in registry changes, even if ic-replica is not using it directly, that it’s gonna affect the build output because the replica is depending on registry package. i.e. replica package output will change for sure, and therefore most likely ic-replica output will too.
Why not? Source control changes should be self-describing. If you have to put a preamble above the proposal summary change log to explain the Git graph, it’s probably because Git isn’t being used in the most effective way. Merging also means you can continue to use a simple ‘change log since commit’ reference (and it would always make sense).
Why would you need to automate it? It’s just normal branch management. The complexity is minimal, and it’s already there, you’d just be handling it upfront rather than pushing the complexity on to a preamble in the proposal summary.
A Git graph that makes sense by itself rather than requiring special mention in proposal summary to justify it
A ‘change log since commit’ reference that’s always accurate and comprehsive (automatically)
For a Web3 outfit I’d say those are pretty big win, and the cost is just sticking to a well structured Git workflow (I’m not suggesting anything exotic).
It’s supposed to be a change log. That’s what the usual ‘change log since’ comment alludes to. There’s already a sensible mechanism for structuring this change log (sections, based on commit message conventions). If you want to optimise for release notes, I don’t see why you’d want to take stuff away (other than because it’s currently hard to get it right). It would be better (if anything) to just add a paragraph at the start that describes the general theme of the proposal, what it seeks to achieve, how it relates to elements of the road map, and maybe what the most important commits are considered to be (there’s no reason this should come at the expense of hiding other commits that modify IC-OS).
I very much disagree. This would be practical if DFINTY were using dedicated release branches instead of trunk based development. But that’s not the case. As it currently stands this approach would yield a huge number of unrelated, unimportant changes, particularly for HostOS releases. DFINITY should be prioritising ways of making this less painful if the aim really is to achieve greater governance decentralisation.
If developers are making changes and they’re not mindful of whether or not those changes modify IC-OS, they surely should be…
The current situation is bad for governance decentralisation (I wrote a whole post about it). What I’m suggesting is a step towards fixing the root of the problem. It’s obvious that something needs to be done (I don’t think that’s contentious). Do you have other suggestions?
@Lorimer don’t get me wrong. i appreciate your input. i’m just trying to give you my view of things.
my main objective is that changes are indeed auditable. i.e. that a person can inspect the differences between latest release and the current one. i’d also like it to be easier for you and anyone else wanting to do this.
at the same time, we also have to consider if we’re as a collective investing more time to try to make it easier to check release notes than actually overcoming the hurdles in checking the changes. e.g., requiring developers to modify their workflow incurs cost and i don’t think in this case would necessarily make things better. similar goes for merging commits into master and then reverting them - it introduces overhead for each release we do. i don’t quite get from your response how much time would this save you or make it easier to review the code. in this particular case, it would also be nice to get some feedback from other members of CodeGov. maybe there’s something i’m not seeing to justify this change.
if you look into commit history of DRE, you can see that i’m trying to improve the release notes generation small steps at a time. i like this approach because it’s making the release notes continually better through code and no human needs to invest more time as a result of changes that i did. if you see some changes that are making things worse, please let me know.
some of that is not solving the real problem - we have some dependency chains that simply need to be untangled. these changes are a serious time investment and as much as i’d like it to be done asap, the truth is that it’s going to take significantly more time to solve it. it’s probably also the case that we’ll always have some screwed up dependencies paths and some of the work done on release controller is going to enable us to fix release notes much faster when that occurs.
i don’t want to discourage you to ask for improvements. i simply ask that you consider if there are any solutions that do not increase the cost of release notes (if you can make PRs to improve release-controller in dre repo, that’s even better). i think once we’re exhausted all options that do not increase the costs, we can then talk about approaches that do.
other than that, i was thinking of implementing some script similar to what we have for checking build determinism to help you identify the commits that modify the IC replica in an easier way. for example, it could print out all the commit links since last release and mark them in some way (e.g. “modifying replica”, “probably not modifying replica”, and “not modifying replica”). let me know what you think of this idea.
it would be doing the same kind of filtering that we already do when generating release notes. except it wouldn’t remove any commits. it would just mark commits indicating if it changed the functionality of the replica or not. e.g. if some tests changed in rs/tests, this is in in dependency path of guestos so it would say “not modifying”. for something like governance canister changes that are currently in dependency path of replica but almost certainly not modifying replica functionality, it would say “probably not modifying”. and last if replica or orchestrator code was changed in the commit, it would mark it as modifying replica.
It sounds like this would be helpful, thanks @Luka. My first thought is why not put this in the release notes so that everyone can benefit. If markdown summary/detail tags are supported, the details that are considered to be unimportant (or likely irrelevant) could be collapsed and hidden by default (allowing a voter to choose to expand it if they’re interested in inspecting)
There’s a decent chance we’d blow past that, but I think we can at least try recording full summaries in this github repo, and then after some time see if we could do it also directly on proposal. These files would then have some crossed out text that would be removed in proposal.
