Upgrade policy #4045
Replies: 2 comments 2 replies
-
Thanks for the detailed write-up, @waiting-for-dev! It sounds like we're trying to tackle the problem both with code and with better changelog practices, which I think is the right way to go. I'll try to leave my thoughts on some of your ideas below.
This doesn't seem so annoying to me. If we want to avoid any confusion stemming from the deprecated defaults, we could consider simply adding a comment explaining that the default value is actually deprecated and that it's going to replaced in the next major version (or that the configuration option will be removed altogether), so that devs inspecting the code have a clear idea of what's going on. An alternative, more complicated (but cooler!) approach could be to do this in code, e.g. preference :address_requires_phone, :boolean, defaults: {
'2.10' => true,
'2.11' => false,
} This would make it clear that the setting has changed over time, and it would also allow us to implement some automated tooling on top of it (e.g., to generate an initializer with the new configuration options since your previous Solidus version). However, I don't think the effort here justifies the advantages. When it comes to configuration management, a As for the changelog, how about having a core team member always update the changelog in the original PR before it's merged? GitHub allows you to update someone else's PR if you enable the feature, and this seems like the perfect use case for doing it. I don't like the idea of pushing to master because it's potentially dangerous and makes reverts (and reconstructing the Git history) harder. |
Beta Was this translation helpful? Give feedback.
-
Had a little internal discussion around this with @adammathys, @forkata, @benjaminwil, and @MadelineCollier. We're basically all on the same page: the |
Beta Was this translation helpful? Give feedback.
-
We talked with @kennyadsl and @cpfergus1 about ideas to provide users with a clear path to upgrade major releases of Solidus. Here it's a wrap-up so that we can start a discussion with the broader community.
We all agreed that the future compatible model we're already following is a good idea. That's, having the last minor version of a major branch deprecating through warnings all the code that won't work in the next major version anymore.
We also have changes in behavior that can support backward compatibility through a setting. At this point, what we're doing is adding a new setting with a default value that keeps the deprecated behavior. However, when a new Solidus app is created, an initializer is generated, setting the configuration to the new recommended value. This way, only fresh installations will use it by default. See #3749 as an example.
This is similar to what Rails are doing. They also add a setting in which the default value keeps the deprecated behavior. For new applications, instead of setting the recommended value in an initializer, they call the
load_defaults
method with the latest Rails version. However, they also provide some guidance to update existing installations. Therails app:update
task creates an initializer where all the new defaults are commented. Users can activate them one by one and, once there're no more items to enable, they can update theload_defaults
call and remove the initializer altogether.Both approaches have a very inconvenient drawback. They leave the core code with a deprecated default value. It makes the code reflect the history that leads to the current snapshot, making things more difficult to understand.
We discussed an alternative approach is to have an update task that creates a
solidus_{OLDER_VERSION}_defaults
initializer instead. In this way, we would also keep backward compatibility behavior while our core code would reflect the current state of recommendations. As in the Rails approach, users would still be able to update step-by-step. Eventually, they could also remove the initializer altogether.This workflow also comes with its drawbacks. To begin with, we'd differing from what Rails is doing. More importantly, users who upgrade the application without running the update task would end up without the initializer and, therefore, breaking their application. On Rails, a blind update leaves you on the safe side. On top of that,
load_defaults
groups settings in chunks, which can be useful for debugging. However, reading the docs for a major upgrade is a very minimum requirement for a developer, and having less complexity on Solidus code is a great benefit.On Rails, the
rails app:update
does more than that. The task tries to update what is needed on the code magically. It does so in attempting to runrails new
with some tweaks on top of the old application. We talked that it's probably a bad idea because it's brittle and encourages users to modify the code blindly. For these kinds of changes, a good upgrade guide is the only reliable option.Ensuring that we provide an excellent upgrade guide is paramount to help users in the process. We considered some options:
Thoughts?
Beta Was this translation helpful? Give feedback.
All reactions