search

oborel / obo-relations

RO is an ontology of relations for use with biological ontologies
http://oborel.github.io/
Other
91 stars 46 forks source link

Fix broken commit history and dangerous project settings #732

Closed cthoyt closed 1 year ago

cthoyt commented 1 year ago

Here's what happened:

  1. 672 was merged, even though CI was failing. Why was it failing: this branch was created before migrating RO to use the ODK #492/#681 and had not been updated since. This meant that 1) the diff was big and confusing and 2) it lacked lots of other quality improvements done since, including switching from DCE to DC Terms (#690/#692).

  2. https://github.com/oborel/obo-relations/pull/730 was created to revert the commit, but never actually applied. @wdduncan you noted in the PR that you applied the revert, but this never changed the main branch
  3. More complicated changes were made and merged in #725, while the main branch was still failing

This leaves us in a place where there are huge, spurious diffs mixed up in bad changes that break CI. It makes it hard to figure out what changes happened and why.

What I suggest we do immediately:

  1. Reset the main branch back to a6d1754
  2. Re-do #730 (sorry @anitacaron)
  3. Change the settings for this repository to make sure that PRs can not be merged if CI is not passing
  4. Fix #731 (it includes tons of spurious diff and is still not passing CI)
wdduncan commented 1 year ago

Yes. I merged the PR w/o noticing that it was failing. That is my fault.

I'm not sure we will be able to fix the commit history.

cthoyt commented 1 year ago

@wdduncan it is technically possible and I know how to do it, but will require some trickery and getting everyone on board with the idea that we should try and keep the commit history clean and meaningful

gouttegd commented 1 year ago

We’re mixing three different problems here:

1) Fixing the state of the master branch, which is currently broken. No argument here, we should absolutely do that. And for that, all we have to do is to merge #731 in its current state. It has been made precisely to fix the mess left by #672, and it does pass the CI checks – it does not need more “fixing“, the “spurious diff“ are there precisely because they correct the spurious diff introduced by #672.

2) Preventing this kind of things from happening in the future. Again, no argument, that’s obviously the correct thing to do.

3) Fixing the history of the master branch, so that it looks like the problem never happened in the first place. This involves re-writing the history to make all the commits of #672 and #725 completely disappear. @diatomsRcool and @anitacaron will have to redo the corresponding work (hoping they have kept it in a local branch somewhere). And whoever has already updated their local copy of the master branch since last Friday will have to fix their own copy of the history before they can update it to the new, re-written public master branch.

Option 3) is more work, more pain, for (in my opinion) no justifiable reason. I am all in favour of a “clean, meaningful history“ (hell, I am the one on record saying that people who write meaningless commit messages should be hung, drawn, and quartered), but history re-writing should only ever happen in private branches. Once the mess reaches a public branch, we must own it instead of going to great lengths to pretend it didn’t happen.

That being said, I don’t have any kind of authority in RO, so if that’s really what we want, then go ahead. But if we do that, we must do it ASAP (like, right now!) – the more time passes, the more likely it is that other people update their local repository with the current master branch.

matentzn commented 1 year ago

I personally think we should protect the master branch now, merge the fix, and leave the git history in whatever state it is in; this is not really like a software project where a broken master can break downstream code (if it was, I would agree that fixing the git history would make some sense).

cthoyt commented 1 year ago

alright, I guess I am outvoted. let's just get this stuff fixed ASAP

balhoff commented 1 year ago

Anyone, please go ahead and merge the fixed PR.

cthoyt commented 1 year ago

This issue can be closed once branch protections are set up on this repo

balhoff commented 1 year ago

@cthoyt do you have permissions to do that? If so that would be great, or else I can do it later once I get to my office.

matentzn commented 1 year ago

I have updated it now, no more merging, not even by maintainers.

gouttegd commented 1 year ago

this is not really like a software project where a broken master can break downstream code (if it was, I would agree that fixing the git history would make some sense).

For the record: no, it would not make any more sense. Whether it is a software project or anything else, if what you care about is “not breaking anything downstream“, then what is important is always the current state of the master branch. Whether the history of the branch contains broken commits is of no consequence – unless some people downstream decide to fork the branch at an arbitrary commit rather than at the tip, but if they do that, frankly it’s up to them to deal with the possibility that the commit they have chosen to fork off may be broken.

anitacaron commented 1 year ago

All settings are set to prevent merging a branch when there's an issue.

anitacaron commented 1 year ago

I ran a new RO release and PR #722 was reverted. Need to redo it.

anitacaron commented 1 year ago

The problem was only partially solved. The only PR that would solve is the one that was never merged, PR #730.