search

OpenHistoricalMap / issues

File your issues here, regardless of repo until we get all our repos squared away; we don't want to miss anything.
Creative Commons Zero v1.0 Universal
17 stars 1 forks source link

Redo ohm-website fork #735

Open 1ec5 opened 4 months ago

1ec5 commented 4 months ago

I discovered in https://github.com/OpenHistoricalMap/issues/issues/505#issuecomment-2036370793 that ohm-website’s en.yml and much of the code that refers to it are suffering from bad merges and ad-hoc patches that probably need to be revisited. Given the effort it would take to fully audit this file again, I wonder if it would actually be more straightforward to start over from scratch, as we did with iD in https://github.com/OpenHistoricalMap/issues/issues/395#issuecomment-1254527931. We’d benefit from a clean slate, easier merges, and a clear path to solving persistent classes of bugs like #442 #443. I’m not completely convinced that we need to take this step urgently, but I’d like to hear what others think of this approach.

We’d create a new branch based on the latest openstreetmap-website, then go through these 439 commits looking for changes we want to keep. https://github.com/OpenHistoricalMap/issues/issues/198#issuecomment-1216889403 started to quantify the changes based on changed files, but I think going commit by commit might be easier, because we’d probably discover a bunch of commits that we don’t want to bring forward. Most of the customizations in iD went through a pull request process, so it was easier to draw up a list of changes we wanted. By contrast, it seems like many changes to the staging branch of ohm-website were committed directly to staging or at least cherry-picked to it.

Unfortunately, I’m not as familiar with the customizations that took place in ohm-website as I was with iD when we decided to redo the fork. But skimming through the commits, I see the following broad areas to focus on:

It may seem daunting to redo 439 commits and changes to 268 files, but the experience with iD shows that the actual meaningful changes make up a much smaller, more manageable subset than a diff would indicate. The alternative would be to keep maintaining the existing technical debt and playing whack-a-mole with bad merges as we discover them.

matkoniecz commented 4 months ago

but I’d like to hear what others think of this approach

this may be a good idea to redo fork, but is it possible to identify what went wrong and avoid the same problems in future? To avoid redoing fork in 2030, this time with 900 commits?

(note: by "identify what went wrong" I do not mean "find people to blame for problems" but find what kind of technical changes, documentation, changes to process and review can be done to avoid such bad merges. Blaming people will not help at all and will just chase away volunteers.)

then go through these 439 commits looking for changes we want to keep

Based on some "fun" I had with managing forks and pulling in upstream changes: this seems manageable?

Especially if there would be some automated process (CI or just script running locally) that would allow to verify that site seems working - tested after applying each change or group of changes?

(I commented as I have some experience with maintaining fork and I want OHM to go well - hopefully it was useful despite no real activity in OHM)

danrademacher commented 4 months ago

I'd like to hear from @erictheise on this.

I'm not opposed to seemingly radical changes like this if it means less tech debt in future. Certainly if we go back to 2017 when we were first finding our way on this work, I am not surprised that there are thing we did then that we'd do differently now.

But also if our main issues are centered on en.yml Id want to confirm in #520 that starting our fork over is the right approach, and then have in place a better system of managing the main locale file and how it is referenced by html.erb files throughout the Rails app.

1ec5 commented 4 months ago

this may be a good idea to redo fork, but is it possible to identify what went wrong and avoid the same problems in future? To avoid redoing fork in 2030, this time with 900 commits?

That’s basically what I’m concerned about. Since we don’t maintain the forks via an explicit series of patches as proposed in #198, the commit history has a lot of noise from commits that are now superseded or weren’t quite right in the first place.

I don’t want to speak for the development team, but my impression was that it started as an upstream change that refactored a lot of strings (and the code that uses them), which was inaccurately merged into staging. Since then, we’ve issued a series of spot fixes, as noted in https://github.com/OpenHistoricalMap/issues/issues/505#issuecomment-2036370793, but each time we only appear to have gotten the end result or tests to match expectations, without aligning the underlying code and identifiers with upstream.

I think these days the team is much more experienced performing these merges and probably would carry out much cleaner merges given a cleaner baseline. But as long as we stay on this branch, we have to tiptoe around the messiness of past merges without the benefit of a clean commit history to consult.

1ec5 commented 4 months ago

But also if our main issues are centered on en.yml Id want to confirm in https://github.com/OpenHistoricalMap/issues/issues/520 that starting our fork over is the right approach, and then have in place a better system of managing the main locale file and how it is referenced by html.erb files throughout the Rails app.

If we have an idea how to move forward on a system like that, it could be beneficial whether or not we rebase our customizations onto the latest openstreetmap-website. I’m wary of resolving the issue without being able to distinguish between customizations we’ve intentionally made and other reasons for diverging from upstream.

The benefit of rebasing is that it forces us to decide which changes to keep in chronological order, without upstream changes mixed into the timeline. It’s the same reason I tend to prefer linear commit history when possible (rebasing instead of merging). In my experience on other kinds of projects, rebasing a long-lived branch such as staging would usually be more trouble than it’s worth, but if there are known bad merges, then the calculus changes significantly for me.

Workflow aside, part of the problem is that openstreetmap-website isn’t a generic, content- and community-agnostic platform. Most of our string customizations are for things that, say, a MediaWiki-powered site wouldn’t even have to fork MediaWiki for. We could lobby upstream for some refactoring, like using a site name variable instead of hard-coding “OpenStreetMap” everywhere and externalizing the list of sources. But there’s probably a limit to how much they’d be willing to do that for the sake of a fork.

matkoniecz commented 4 months ago

We could lobby upstream for some refactoring, like using a site name variable instead of hard-coding “OpenStreetMap” everywhere

Well, https://github.com/openstreetmap/openstreetmap-website is " The Rails application that powers OpenStreetMap " not "generic platform to host crowdscourced maps", and there are 121 waiting pull requests.

I would not expect this to be high on priority list.

(though I know that some obsolete functionality was not removed as doing this would harm OHM and Geofiction - while it was unused by OSM)

1ec5 commented 4 months ago

I’m not holding my breath either. 😅 Maybe we could simplify our fork by upstreaming certain customizations. For example, our Leaflet–MapLibre compatibility shim would come in handy for OSM’s Year of Vector Maps, so that OSM wouldn’t need to roll their own implementation once the tileset and stylesheet are ready. That would enable us to collaborate more effectively on a subsequent true migration to MapLibre GL JS without diverging significantly: #651. But none of that can take place until we rebase.