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

Handle automatically all settings sync conflicts (BlueOS vs Cockpit) #1684

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rafaellehmkuhl
Copy link
Member

@rafaellehmkuhl rafaellehmkuhl commented Feb 10, 2025

This PR removes completely removes the need for the user to solve sync conflicts. All of those are now solved automatically based on change-timestamps.

With it, we always preserve the most recent change, considering of course that this change belongs to the same user and vehicle.

If one performs changes while disconnected, the changes will be preserved and pushed to the vehicle in the next boot as long as the user connects to the same vehicle as it was previously, otherwise we pull the changes from the new vehicle.

For the reviewers: this patch changes a sensitive pipeline of Cockpit, so it's important that all reviewing test the functionality as well. The tests that should be performed consist on changing vehicles and users. Settings should always be kept in sync with the same user/vehicle combination.

There should be only one situation (user connected to vehicle 1 > user disconnects > user makes changes to their settings > user connects to vehicle 2) where someone changing their settings while offline loses those changes. I've opened issue #1686 to handle that as well, but it's a more rare edge-case and depends on some changes I would prefer not doing now so I don't mess with the changes being done on #1685.

Fix #1627
Fix #1628

Closes all of those related to solving conflicts as well, as we don't have conflicts to deal with anymore.

Fix #1120
Fix #1383
Fix #1626

@rafaellehmkuhl rafaellehmkuhl force-pushed the automatically-solve-almost-all-settings-sync-conflicts branch 3 times, most recently from e7e0806 to f9cba2a Compare February 10, 2025 17:34
@rafaellehmkuhl rafaellehmkuhl changed the title [WIP] Handle ~almost~ all settings sync conflicts (BlueOS vs Cockpit) automatically [WIP] Handle automatically ~almost~ all settings sync conflicts (BlueOS vs Cockpit) Feb 10, 2025
@rafaellehmkuhl rafaellehmkuhl force-pushed the automatically-solve-almost-all-settings-sync-conflicts branch from f9cba2a to 9156204 Compare February 11, 2025 17:22
@rafaellehmkuhl rafaellehmkuhl changed the title [WIP] Handle automatically ~almost~ all settings sync conflicts (BlueOS vs Cockpit) Handle automatically ~almost~ all settings sync conflicts (BlueOS vs Cockpit) Feb 11, 2025
@rafaellehmkuhl rafaellehmkuhl changed the title Handle automatically ~almost~ all settings sync conflicts (BlueOS vs Cockpit) Handle automatically all settings sync conflicts (BlueOS vs Cockpit) Feb 11, 2025
@rafaellehmkuhl rafaellehmkuhl marked this pull request as ready for review February 11, 2025 17:35
Copy link
Contributor

@ArturoManzoli ArturoManzoli left a comment

Choose a reason for hiding this comment

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

Nice approach to the configs priority!
No conflicts showed up

- On Cockpit Web -
While testing the persistence of the setup changes, I started with a fresh run of the Cockpit, with default settings, no user or vehicle configured.

I created a new user, changed the glass color and then connected to the vehicle;
The user was saved on the vehicle, as it should be but the glass color didn't persist after the connection and the restarts;

Also, I had some trouble with cockpit hanging after connected to the vehicle. I'll try to post the console log.
When opening the general settings to change the user after connecting and restarting, the user selection dialog didn't show up; Then after a new boot, cockpit hung after opening the main menu; Then , on the third restart Cockpit behaved normally.

Will do the same test later on Electron.

Screenshare.-.2025-02-11.7_16_34.PM.mp4

@ArturoManzoli
Copy link
Contributor

Here is the problem mentioned above with the console log

Screenshare.-.2025-02-11.7_29_54.PM.mp4

@ArturoManzoli
Copy link
Contributor

ArturoManzoli commented Feb 12, 2025

Also checked on master and Cockpit doesn't have the same issue over there

@ES-Alexander
Copy link
Contributor

ES-Alexander commented Feb 13, 2025

Closes #1674 as well as we don't have conflicts to deal with anymore.

@rafaellehmkuhl This seems like an incorrect link.

Did you mean #1120, #1383, #1626 or something else?

@rafaellehmkuhl
Copy link
Member Author

Closes #1674 as well as we don't have conflicts to deal with anymore.

@rafaellehmkuhl This seems like an incorrect link.

Did you mean #1120, #1383, #1626 or something else?

The point is that there will be no more conflicts to solve, so all of those will be closed as well. #1674 as well as the ones you mentioned. Will add them to the PR description.

@rafaellehmkuhl rafaellehmkuhl force-pushed the automatically-solve-almost-all-settings-sync-conflicts branch from 9156204 to 7999c6f Compare February 13, 2025 16:46
@rafaellehmkuhl rafaellehmkuhl force-pushed the automatically-solve-almost-all-settings-sync-conflicts branch from 7999c6f to 15ca583 Compare February 13, 2025 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants