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.

Sign up for GitHub

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

Jump to bottom

use correct version copmparison on NSIS updater: fix update from rc #4979

Merged
merged 3 commits into from
Oct 12, 2022
Merged

use correct version copmparison on NSIS updater: fix update from rc #4979

merged 3 commits into from
Oct 12, 2022

Conversation

mgallien
Copy link
Collaborator

@mgallien mgallien commented Sep 27, 2022 •
edited by tobiasKaminsky
Loading

@mgallien mgallien force-pushed the bugfix/fixVersionComparisonsOnWindows branch 2 times, most recently from 9d461f2 to 80b1b35 Compare September 28, 2022 06:22
@mgallien mgallien mentioned this pull request Sep 28, 2022
Closed
allexzander
allexzander reviewed Sep 28, 2022
View reviewed changes
src/gui/updater/ocupdater.cpp Outdated Show resolved Hide resolved
src/gui/updater/ocupdater.cpp Outdated Show resolved Hide resolved
@allexzander allexzander self-requested a review September 28, 2022 07:47
allexzander
allexzander suggested changes Sep 28, 2022
View reviewed changes
Copy link
Contributor

@allexzander allexzander left a comment

Choose a reason for hiding this comment

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

@matthieugallien
Besides some nitpicks, I've got question:

What about the feature of skipping a certain version? I see that now seenVersion is no longer in use, is this expected? Also, if this is the case, the entire code for seenVersion needs to be removed as well to avoid confusion?

@codecov
Copy link

codecov bot commented Sep 28, 2022 •
edited
Loading

Codecov Report

Merging #4979 (a36672c) into master (bde2f90) will decrease coverage by 0.02%.
The diff coverage is n/a.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #4979      +/-   ##
==========================================
- Coverage   57.34%   57.31%   -0.03%     
==========================================
  Files         138      138              
  Lines       17391    17391              
==========================================
- Hits         9972     9967       -5     
- Misses       7419     7424       +5
Impacted Files Coverage Δ
src/libsync/propagatedownload.cpp 63.58% <0.00%> (-1.03%) ⬇️
src/libsync/propagatorjobs.cpp 73.23% <0.00%> (+1.40%) ⬆️

@mgallien mgallien force-pushed the bugfix/fixVersionComparisonsOnWindows branch from 80b1b35 to 8dc8f55 Compare September 28, 2022 09:41
@mgallien
Copy link
Collaborator Author

@matthieugallien Besides some nitpicks, I've got question:

What about the feature of skipping a certain version? I see that now seenVersion is no longer in use, is this expected? Also, if this is the case, the entire code for seenVersion needs to be removed as well to avoid confusion?

I am not convinced we should fix the current interface to tell users about the skipped version. I would prefer to remove it altogether as we should aim to produce new versions that are not broken and skipping an update should never be needed.

@mgallien mgallien force-pushed the bugfix/fixVersionComparisonsOnWindows branch from 8dc8f55 to aca16d5 Compare September 29, 2022 19:53
@mgallien mgallien force-pushed the bugfix/fixVersionComparisonsOnWindows branch from aca16d5 to 71d9284 Compare October 5, 2022 07:58
@mgallien mgallien force-pushed the bugfix/fixVersionComparisonsOnWindows branch 3 times, most recently from 2d05827 to ac32865 Compare October 12, 2022 07:49
@mgallien
remove support for skipping an update: hard to understand for users
ad10d4b 
skiping an update is probably there for historical reasons to work
around broken updates in the past

this can lead to users not getting an update they should be getting

this can elad to user confusion and the current user interaction is
broken if you do not go read the settings file with a text editor

Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
@mgallien
use correct version copmparison on NSIS updater: fix update from rc
146bd44 
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
@mgallien
fix review comments
a36672c 
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
@mgallien mgallien force-pushed the bugfix/fixVersionComparisonsOnWindows branch from ac32865 to a36672c Compare October 12, 2022 10:13
@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-4979-a36672c1a4c13d50017c2c5629d3a4e5e55131b9-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

36.4% 36.4% Coverage
0.0% 0.0% Duplication

@mgallien mgallien merged commit 25c64b0 into master Oct 12, 2022
@mgallien mgallien deleted the bugfix/fixVersionComparisonsOnWindows branch October 12, 2022 11:43
@mgallien mgallien added this to the 3.7.0 milestone Oct 18, 2022
@mgallien
Copy link
Collaborator Author

/backport to stable-3.6

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Oct 18, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@allexzander allexzander allexzander approved these changes

@camilasan camilasan Awaiting requested review from camilasan

@claucambra claucambra Awaiting requested review from claucambra

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.7.0
Development

Successfully merging this pull request may close these issues.

Deskop does not offer 3.6.0 stable
3 participants
@mgallien @nextcloud-desktop-bot @allexzander