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

Implement upgrade_only_if_version_changes in manifest #864

Merged
merged 32 commits into from
Sep 3, 2020

Conversation

Josue-T
Copy link
Contributor

@Josue-T Josue-T commented Dec 30, 2019

The problem

Solution

Implement in the core this concept

Add helper ynh_compare_package_version.

PR Status

Work done

How to test

  • Install an app.
  • Add "upgrade_only_if_version_changes":true in the manifest
  • Upgrade an app in debug mode and see that you will have environment variable named YNH_APP_UPGRADE_TYPE which will give you the type of upgrade. It could be:
    • UPGRADE_FORCED when there are nothing to update but the user forced the update
    • UNKNOWN for the app which don't use the correct syntax for the app version management.
    • UPGRADE_FULL when the upstream and the package need an update.
    • UPGRADE_APP when the upstream need to be updated.
    • UPGRADE_PACKAGE when the package need to be updated.
  • You will see that no upgrade will be done if there are nothing to update but you still can fore this with -F.
  • Try the helper ynh_compare_package_version in the upgrade of an app.

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :

@Josue-T Josue-T changed the title Abort if up to date manifest Implement upgrade_only_if_version_changes manifest Dec 30, 2019
@Josue-T Josue-T changed the title Implement upgrade_only_if_version_changes manifest Implement upgrade_only_if_version_changes in manifest Dec 30, 2019
@Josue-T Josue-T requested review from a team, alexAubin, kay0u and maniackcrudelis April 13, 2020 15:52
@maniackcrudelis
Copy link
Contributor

And globally, add fucking comments !!!
We shouldn't have to try to figure out how it works !

Josue-T and others added 5 commits April 14, 2020 13:45
src/yunohost/app.py Show resolved Hide resolved
src/yunohost/app.py Outdated Show resolved Hide resolved
src/yunohost/app.py Outdated Show resolved Hide resolved
data/helpers.d/utils Outdated Show resolved Hide resolved
Josue-T and others added 5 commits April 15, 2020 11:53
Co-Authored-By: Alexandre Aubin <alex.aubin@mailoo.org>
Co-Authored-By: Alexandre Aubin <alex.aubin@mailoo.org>
src/yunohost/app.py Outdated Show resolved Hide resolved
src/yunohost/app.py Outdated Show resolved Hide resolved
Copy link
Member

@alexAubin alexAubin left a comment

Choose a reason for hiding this comment

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

Apart from remaining small comments to improve code, that looks good to me ! Thanks for bearing with the question / suggestions 😅

src/yunohost/app.py Outdated Show resolved Hide resolved
src/yunohost/app.py Outdated Show resolved Hide resolved
src/yunohost/app.py Outdated Show resolved Hide resolved
src/yunohost/app.py Outdated Show resolved Hide resolved
src/yunohost/app.py Outdated Show resolved Hide resolved
Josue-T and others added 5 commits April 27, 2020 11:05
Co-Authored-By: Alexandre Aubin <alex.aubin@mailoo.org>
Co-Authored-By: Alexandre Aubin <alex.aubin@mailoo.org>
Co-Authored-By: Alexandre Aubin <alex.aubin@mailoo.org>
Copy link
Member

@alexAubin alexAubin left a comment

Choose a reason for hiding this comment

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

LGTM

@SilverViper
Copy link

SilverViper commented May 2, 2020

Fixes #33 ?

Edit: link because markdown on mobile is flawed:
YunoHost/issues#33

src/yunohost/app.py Show resolved Hide resolved
@alexAubin alexAubin changed the base branch from stretch-unstable to 4.1 August 14, 2020 15:26
@alexAubin
Copy link
Member

c.f. a quick discussion on the chat : I'm proposing to enable this behavior by default, without adding/enabling the option in every manifest app (which is going to be a pain in the ass because we have 140+ apps and hmpf)

To be discussed during tomorrow's meeting

-F:
full: --force
help: Force the update, even though the app is up to date
action: store_true
Copy link
Member

Choose a reason for hiding this comment

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

I suggest a break change:

-f is used in install operation as in other part of the cli. I suggest to define -F for --file and -f for --force.

This change concerns packagers, and i prefer a break change for packager, than an inconsistency for final users

Copy link
Member

Choose a reason for hiding this comment

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

N.B. : it's used here : https://github.com/YunoHost/package_check/blob/master/sub_scripts/testing_process.sh#L965 so gotta be careful

I'm okay with the change but maybe we can do this in a separate PR to move forward ...

@alexAubin alexAubin merged commit b869f3a into 4.1 Sep 3, 2020
@alexAubin alexAubin deleted the abort_if_up_to_date_manifest branch September 3, 2020 13:51
@alexAubin alexAubin added this to the 4.1.x milestone Sep 3, 2020
@zamentur zamentur added this to In progress in 4.1.x via automation Jan 4, 2021
@zamentur zamentur moved this from In progress to Done in 4.1.x Jan 4, 2021
@zamentur zamentur removed this from the 4.1.x milestone Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
4.1.x
  
Done
7 participants