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

Pass an additional parameter on the core update #32491

Merged
merged 1 commit into from
Nov 23, 2018
Merged

Pass an additional parameter on the core update #32491

merged 1 commit into from
Nov 23, 2018

Conversation

VicDeo
Copy link
Member

@VicDeo VicDeo commented Aug 28, 2018 •
edited
Loading

Description

pass the additional argument that shows whether major core update in progress

Related Issue

Motivation and Context

Minor ownCloud upgrades should not trigger minor app updates

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • Pass --major CLI option into the repair step
  • Backport (if applicable set "backport-request" label and remove when the backport was done)

@VicDeo VicDeo added this to the development milestone Aug 28, 2018
@VicDeo VicDeo self-assigned this Aug 28, 2018
@CLAassistant
Copy link

CLAassistant commented Aug 28, 2018 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@VicDeo VicDeo mentioned this pull request Aug 28, 2018
Merged
6 tasks
@codecov
Copy link

codecov bot commented Aug 28, 2018 •
edited
Loading

Codecov Report

Merging #32491 into master will decrease coverage by 0.74%.
The diff coverage is 38.46%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #32491      +/-   ##
============================================
- Coverage     64.22%   63.48%   -0.75%     
- Complexity    18269    18274       +5     
============================================
  Files          1193     1193              
  Lines         69077    69078       +1     
  Branches       1277     1277              
============================================
- Hits          44364    43852     -512     
- Misses        24341    24854     +513     
  Partials        372      372
Flag Coverage Δ Complexity Δ
#javascript 52.91% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 64.7% <38.46%> (-0.83%) 18274 <5> (+5)
Impacted Files Coverage Δ Complexity Δ
lib/private/Repair.php 25.88% <ø> (+0.3%) 21 <0> (ø) ⬇️
core/Command/Upgrade.php 0% <0%> (ø) 68 <0> (+1) ⬆️
lib/private/Updater.php 12.58% <0%> (-1.37%) 49 <2> (+2)
lib/private/Repair/Apps.php 24.84% <88.23%> (+17.56%) 46 <3> (+2) ⬆️
lib/private/DB/PostgreSqlMigrator.php 0% <0%> (-100%) 6% <0%> (ø)
lib/private/DB/SQLiteSessionInit.php 0% <0%> (-100%) 4% <0%> (ø)
lib/private/DB/SQLiteMigrator.php 0% <0%> (-100%) 7% <0%> (ø)
...ilder/ExpressionBuilder/PgSqlExpressionBuilder.php 0% <0%> (-88.89%) 3% <0%> (ø)
lib/private/DB/AdapterOCI8.php 0% <0%> (-86.67%) 4% <0%> (ø)
...ivate/Files/ObjectStore/HomeObjectStoreStorage.php 0% <0%> (-86.67%) 8% <0%> (ø)
... and 41 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6501e8...7a021be. Read the comment docs.

@VicDeo VicDeo mentioned this pull request Aug 28, 2018
Closed
7 tasks
@mmattel
Copy link
Contributor

mmattel commented Aug 28, 2018

Documentation relevant!
Please raise a ticket there to be prepared
@settermjd

PVince81
PVince81 reviewed Nov 9, 2018
View reviewed changes
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

Code looks good. Please add unit test for the event

@VicDeo
Copy link
Member Author

VicDeo commented Nov 9, 2018

@PVince81 this part is still under development.

@VicDeo VicDeo force-pushed the market-387 branch 7 times, most recently from 89dc3ef to 9715514 Compare November 19, 2018 19:48
@VicDeo
Copy link
Member Author

VicDeo commented Nov 19, 2018 •
edited
Loading

@PVince81 rebased, squashed, added test

@VicDeo VicDeo force-pushed the market-387 branch 2 times, most recently from 11efcb0 to b5c82d0 Compare November 21, 2018 12:33
@VicDeo
Copy link
Member Author

VicDeo commented Nov 21, 2018

@ownclouders rebase

@ownclouders
Copy link
Contributor

Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently ⚠️

@ownclouders
Copy link
Contributor

Automated rebase with GitMate.io was successful! 🎉

@VicDeo VicDeo force-pushed the market-387 branch 2 times, most recently from 9da1e75 to cfa6f19 Compare November 22, 2018 09:52
PVince81
PVince81 reviewed Nov 22, 2018
View reviewed changes
core/Command/Upgrade.php
$updater = new Updater(
$this->config,
\OC::$server->getIntegrityCodeChecker(),
$this->logger
);

if ($input->getOption('major')) {
$updater->setForceMajorUpgrade(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

as discussed 👍

PVince81
PVince81 approved these changes Nov 22, 2018
View reviewed changes
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍 see comment

lib/private/Updater.php
* Check if we have empty app folders or incompatible apps enabled
*/
private function checkAppsCompatibility() {
$dispatcher = \OC::$server->getEventDispatcher();
Copy link
Contributor

Choose a reason for hiding this comment

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

not injectable I guess ? I seem to remember the potential for infinite loops

Copy link
Member Author

Choose a reason for hiding this comment

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

TBH, I haven't checked the possibility as I found EventDispatcher already used 3 times like this in this class.

@PVince81 PVince81 merged commit 2f2f3f6 into master Nov 23, 2018
@PVince81 PVince81 deleted the market-387 branch November 23, 2018 15:09
@PVince81
Copy link
Contributor

please backport

This was referenced Nov 23, 2018
Closed
Merged
@VicDeo VicDeo mentioned this pull request Nov 23, 2018
Merged
5 tasks
@VicDeo
Copy link
Member Author

VicDeo commented Nov 23, 2018

Stable10: #33641

@lock lock bot locked as resolved and limited conversation to collaborators Nov 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@PVince81 PVince81 PVince81 approved these changes

Assignees

@VicDeo VicDeo

Labels
3 - To Review
Projects
None yet
Milestone
development
Development

Successfully merging this pull request may close these issues.

Differentiate between minor and major updates
5 participants
@VicDeo @CLAassistant @mmattel @ownclouders @PVince81