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

Differentiate between minor and major updates #387

Closed
7 tasks done
PVince81 opened this issue Aug 6, 2018 · 29 comments · Fixed by owncloud/core#32491
Closed
7 tasks done

Differentiate between minor and major updates #387

PVince81 opened this issue Aug 6, 2018 · 29 comments · Fixed by owncloud/core#32491

Comments

@PVince81
Copy link
Contributor

PVince81 commented Aug 6, 2018

  • adjust automated update:
    • when doing minor update of core, don't fetch major version updates of app from marketplace
    • when doing major update of core, fetch major version updates of app from marketplace
  • admin must have a way to force major update of app
    • occ upgrade --major
    • occ market update --major
  • web UI shows both minor and major update, default on minor
@PVince81 PVince81 added this to the backlog milestone Aug 6, 2018
@PVince81 PVince81 modified the milestones: backlog, development Aug 7, 2018
@VicDeo
Copy link
Member

VicDeo commented Aug 7, 2018

Without saying that it is an overcomplication (just thinking so)

@PVince81 what if a major update is the only available for the new minor core version?

@PVince81
Copy link
Contributor Author

PVince81 commented Aug 8, 2018

@IljaN in case we need something on marketplace as well

@PVince81
Copy link
Contributor Author

PVince81 commented Aug 8, 2018

maybe there's a better library for semver instead of using compare_version

@patrickjahns
Copy link
Contributor

@PVince81 - ask and you shall be given - https://packagist.org/packages/composer/semver

@PVince81
Copy link
Contributor Author

PVince81 commented Aug 8, 2018

what if a major update is the only available for the new minor core version?

@VicDeo I'd be tempted to say: then we don't update it.

This would imply that for every major update of OC there will be a major update for apps as well. I think this makes sense due to compatibility purposes and branching purposes.

@VicDeo
Copy link
Member

VicDeo commented Aug 8, 2018

What about market web UI - should it also show only minor updates as available?

and another thing: I foresee the number of unsuccessful update reports from Web updater users to increase.

@VicDeo
Copy link
Member

VicDeo commented Aug 8, 2018

TBH I don't see any benefit from semver package unless it would be adopted on the core level and wrapped with method isMajorUpdate

@PVince81
Copy link
Contributor Author

PVince81 commented Aug 9, 2018

What about market web UI - should it also show only minor updates as available?

Hmmm, it should show minor updates only by default. But an admin should have a mechanism to force a major update of an app if they want to (if said major update is compatible with current OC).
This would be the equivalent of occ market update --major

@PVince81
Copy link
Contributor Author

@VicDeo any update ? estimate ?

@VicDeo
Copy link
Member

VicDeo commented Aug 24, 2018

@PVince81
Should be easy to implement for backend by passing an additional option to https://github.com/owncloud/market/blob/master/lib/MarketService.php#L194

for web UI part estimates are unclear, all these Vue things look like a summoning the Dark Priest Cthulhu to me...

@VicDeo
Copy link
Member

VicDeo commented Aug 24, 2018

here is a proposed solution:

  1. Add an additional argument to the events dispatched by the core repair step https://github.com/owncloud/core/blob/master/lib/private/Repair/Apps.php#L219

  2. the value is taken from occ upgrade argument or by comparing the versions if the argument is not present...
    what about the web updater case??? this is why I added the second statement into Differentiate between minor and major updates #387 (comment) :/

  3. Process the event argument within the event handler in a market app. If the argument is missed consider it to be a minor update

  4. pass it as a bool parameter isMinorUpdate all the way down into https://github.com/owncloud/market/blob/master/lib/MarketService.php#L194

  5. filter out all compatible major updates if isMinorUpdate is true

  6. add the same occ market update and occ market install
    looks pretty easy so far...

  7. for the web UI we will need calling getAvailableUpdateVersion with isMinorUpdate === true and then with isMinorUpdate === false. The controller will provide both major and minor versions to the frontend instead of the single update candidate

  8. Frontend will pass exact version to the backend depending on the selection.

  9. If both major and minor update is available - minor is default in the web UI

  10. If only major or minor version is available - it is the only (and default) in the web UI

@PVince81
Copy link
Contributor Author

you can grab @felixheidecke for all things Vue wizardry.

@PVince81
Copy link
Contributor Author

@VicDeo the plan sounds good.

Sounds like web updater will also need an option there, maybe a checkbox of some sorts. I suggest opening a ticket in the web updater repo for its plan and task.

@VicDeo
Copy link
Member

VicDeo commented Aug 28, 2018

@PVince81 what do you think on this approach?
owncloud/core#32491
#391

It is missing CLI parameters and frontend part.
As for market install I think it should always install the most recent version by default

@PVince81
Copy link
Contributor Author

looks good

currently I still have an uncomfortable feeling as I don't have enough certainty about what could go wrong and how we can protect against this

based on this approach we could actually release the market app. Wwe need to release it anyway before a core release as core release script fetches the market app from marketplace.

Can you make the flag default to something so that if the matching core version isn't there yet, the updates still work as they did before the change ? Unless we decide to enforce min-version=10.0.10 in the new market app.

@VicDeo
Copy link
Member

VicDeo commented Aug 29, 2018

@PVince81
it has a default - missing argument means we run the market app with an older core and $isMajorUpdate = false; in this case
https://github.com/owncloud/market/pull/391/files#diff-28826ce644d890bd5ffbda75582f2c53R50

@PVince81 PVince81 modified the milestones: development, backlog Sep 27, 2018
@PVince81
Copy link
Contributor Author

TBH I don't see any benefit from semver package unless it would be adopted on the core level and wrapped with method isMajorUpdate

semver package is coming anyway in owncloud/core#31103

@PVince81
Copy link
Contributor Author

Possible scenarios:

Let A be an app with current version 1.0.1 and updatable to either version 1.2.3 (minor) or version 2.0.2 (major)

  1. Major update scenario: OC 10.0.10 -> OC 11.0 (major)
    a) occ upgrade => A is updated to 2.0.2 since the core version is a major update
    b) occ upgrade --major same as a)
    c) occ market update A => A is updated to 1.2.3 as we're updating the app outside of a core update
    d) occ market update A --major => A is updated to 2.0.2
  2. Minor update scenario: OC 10.0.10 -> OC 10.0.11 (minor)
    a) occ upgrade => A is updated to 1.2.3 since the core version is a minor update
    b) occ upgrade --major => A is force-updated to next major 2.0.2
    c) occ market update A => A is updated to 1.2.3 as we're updating the app outside of a core update
    d) occ market update A --major => A is updated to 2.0.2

Ok, this was only to illustrate the scenarios and at this level I don't see anything that could go wrong.

@PVince81 PVince81 modified the milestones: backlog, development Oct 23, 2018
@VicDeo
Copy link
Member

VicDeo commented Oct 24, 2018

for the web UI we will need calling getAvailableUpdateVersion with isMinorUpdate === true and then with isMinorUpdate === false. The controller will provide both major and minor versions to the frontend instead of the single update candidate

not that easy... it's better to refactor market app to deliver update candidates as

appId => [
  'major' => (string) version,
  'minor' => (string) version
]

instead of the current appId=> (string) version

there is too many magic behind the scene so it is better to do it properly

@PVince81
Copy link
Contributor Author

@VicDeo market or marketplace ?

I think the market app controller should prefilter, either return major candidates or minor, not both. There is no use for the frontend for both results, unless you want to implement the filter on the frontend side ?

@VicDeo
Copy link
Member

VicDeo commented Oct 25, 2018

@PVince81 market.
If not both, what should be in UI then? major or minor and why?

@PVince81
Copy link
Contributor Author

It all depends how this is to be implemented.

Approach 1:

  • service / controller returns both versions
  • frontend filters both based on selected mode

Approach 2:

  • service / controller only returns one version based on specified mode (query arg)
  • frontend shows whatever the controller returns
  • when switching mode in frontend, requery the controller with the updated mode

In the end the user will see the same. I thought filtering on backend could be more efficient, especially once we start having a lot more app. Maybe eventually it's the marketplace itself (not market app) that will be able to filter.

@PVince81
Copy link
Contributor Author

  • write / highlight test plan for OC <= 10.0.10 with new market app and future

@VicDeo
Copy link
Member

VicDeo commented Nov 16, 2018

Summary

Notes on testing with core master branch

Our marketplace has no apps for OC 11 platform yet
So this line https://github.com/owncloud/market/blob/master/lib/VersionHelper.php#L40 needs to be replaced with $v = [10, 0, 10,0]; or smth like this

otherwise you'll need an own marketplace that provides apps for OC 11

this part depends on owncloud/core#32491 merged into master.

Core upgrade

  • major (10 to 11 and like this) still DOES upgrade apps from 1.0 to 2.0 (if available)
  • minor (10 to 10.1 and like this) does NOT apps upgrade from 1.0 to 2.0

this part is merged with #391

CLI

  • market:upgrade still DOES upgrade 1.0 to 1.1
  • market:upgrade does NOT upgrade 1.0 to 2.0
  • market:upgrade --major DOES upgrade 1.0 to 2.0
  • market:install does NOT upgrade 1.0 to 2.0 if the app is already installed
  • market:install still DOES upgrade 1.0 to 1.1 if the app is already installed

Market app Web UI

  • Shows both 1.1 and 2.0 for 1.0, 1.1 is default

Market app cron job

  • Generates notifications both for 1.1 and 2.0

Screenshots

Updates tab

newupdatespage

App Page

apppage

Examples

update the app from v0.3.5 to v0.3.5

deo@jah-mobile:> php occ market:upgrade --local ../customgroups.tar.gz 
customgroups: ../customgroups.tar.gz has the same or older version of the app

0.3.5 to 1.3.5 (no --major option specified)

deo@jah-mobile:> php occ market:upgrade --local ../customgroups.tar.gz 
customgroups: ../customgroups.tar.gz has a different major version, try with --major option

0.3.5 to 1.3.5 (with --major)

deo@jah-mobile:> php occ market:upgrade --local ../customgroups.tar.gz --major
customgroups: Installing new version from ../customgroups.tar.gz

@PVince81
Copy link
Contributor Author

@davitol see above whether this is clear enough for testing

@phil-davis phil-davis changed the title Differenciate between minor and major updates Differentiate between minor and major updates Nov 20, 2018
@PVince81 PVince81 mentioned this issue Nov 22, 2018
24 tasks
@PVince81 PVince81 modified the milestones: development, QA Nov 22, 2018
@PVince81
Copy link
Contributor Author

moved to QA to test against v10.0.10 (last stable version) and stable10 (future version which comes with owncloud/core#32491)

@pmaier1
Copy link
Contributor

pmaier1 commented Dec 10, 2018

Did this change find it's way into the docs? Didn't find anything about occ upgrade updating apps at all.

@PVince81
Copy link
Contributor Author

@pmaier1 no.

@VicDeo please raise docs ticket and link to the explanation about how it works (I think we had it in the test plan)

@VicDeo
Copy link
Member

VicDeo commented Dec 10, 2018

owncloud/docs#449

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants