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

Remove apps that will go to market from shipped.json #27985

Merged
merged 2 commits into from May 24, 2017

Conversation

Projects
None yet
4 participants
@PVince81
Member

PVince81 commented May 23, 2017

Description

Makes it possible to uninstall them.

Related Issue

#27983

Motivation and Context

How Has This Been Tested?

  • TEST: setup works
  • TEST: apps from the list still work
  • TEST: occ ugprade doesn't complain, still works (no auto-disable anyway)
  • TEST: removed apps can be uninstalled now #28001

Screenshots (if appropriate):

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)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

I don't have full understanding of the extent of the "shipped" flag. Grepping for isShipped seems to show a lot of locations, even some related to API responses.

@tomneedham @DeepDiver1975 @davitol

Remove apps that will go to market from shipped.json
Makes it possible to uninstall them.

@PVince81 PVince81 added this to the 10.0.2 milestone May 23, 2017

@davitol

This comment has been minimized.

Show comment
Hide comment
@davitol

davitol May 23, 2017

Contributor

Tested and WFM 👍

Contributor

davitol commented May 23, 2017

Tested and WFM 👍

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 23, 2017

Member

Thanks. Don't merge yet, there could be consequences due to the weird OCS code that rely on this flag.

Needs to be evaluated

Member

PVince81 commented May 23, 2017

Thanks. Don't merge yet, there could be consequences due to the weird OCS code that rely on this flag.

Needs to be evaluated

@tomneedham

This comment has been minimized.

Show comment
Hide comment
@tomneedham

tomneedham May 23, 2017

Member

So grepping all of these apps to show what OCS routes they might register:

./activity/appinfo/routes.php://API::register(
./activity/appinfo/routes.php://API::register(
./activity/appinfo/routes.php:API::register(
 ./notifications/appinfo/routes.php:\OCP\API::register(
./notifications/appinfo/routes.php:\OCP\API::register(
./notifications/appinfo/routes.php:\OCP\API::register(
./notifications/tests/Integration/app/appinfo/routes.php:\OCP\API::register(
./notifications/tests/Integration/app/appinfo/routes.php:\OCP\API::register(
./sharepoint/appinfo/routes.php:OC_API::register('get', '/apps/sharepoint/api/v1/mounts',
Member

tomneedham commented May 23, 2017

So grepping all of these apps to show what OCS routes they might register:

./activity/appinfo/routes.php://API::register(
./activity/appinfo/routes.php://API::register(
./activity/appinfo/routes.php:API::register(
 ./notifications/appinfo/routes.php:\OCP\API::register(
./notifications/appinfo/routes.php:\OCP\API::register(
./notifications/appinfo/routes.php:\OCP\API::register(
./notifications/tests/Integration/app/appinfo/routes.php:\OCP\API::register(
./notifications/tests/Integration/app/appinfo/routes.php:\OCP\API::register(
./sharepoint/appinfo/routes.php:OC_API::register('get', '/apps/sharepoint/api/v1/mounts',
@tomneedham

This comment has been minimized.

Show comment
Hide comment
@tomneedham

tomneedham May 23, 2017

Member

My only concern at the moment is if apps are enhancing the notification or activity apis - looking for usages...

Member

tomneedham commented May 23, 2017

My only concern at the moment is if apps are enhancing the notification or activity apis - looking for usages...

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 23, 2017

Member

AFAIK none of our apps are expecting more than a single response block.

Does it make a difference is shipped is there or not ? Does it influence the order or name of the response block ?

Member

PVince81 commented May 23, 2017

AFAIK none of our apps are expecting more than a single response block.

Does it make a difference is shipped is there or not ? Does it influence the order or name of the response block ?

Show outdated Hide outdated core/shipped.json
"files_videoplayer",
"firewall",
"firstrunwizard",
"gallery",
"market",

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 May 23, 2017

Member

Kill market as well

@DeepDiver1975

DeepDiver1975 May 23, 2017

Member

Kill market as well

This comment has been minimized.

@PVince81

PVince81 May 23, 2017

Member

this will allow uninstallling the market app, (P)roceed, (A)bort ?

@PVince81

PVince81 May 23, 2017

Member

this will allow uninstallling the market app, (P)roceed, (A)bort ?

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 May 23, 2017

Member

If somebody wants to....

@DeepDiver1975

DeepDiver1975 May 23, 2017

Member

If somebody wants to....

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 24, 2017

Member

My only concern at the moment is if apps are enhancing the notification or activity apis - looking for usages...

Enhancing ? How ?

Member

PVince81 commented May 24, 2017

My only concern at the moment is if apps are enhancing the notification or activity apis - looking for usages...

Enhancing ? How ?

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 24, 2017

Member

and why

Member

PVince81 commented May 24, 2017

and why

@tomneedham

This comment has been minimized.

Show comment
Hide comment
@tomneedham

tomneedham May 24, 2017

Member

Enhancing ? How ?

well by latching onto the same route and altering / changing the response in some weird way

and why

no idea

Member

tomneedham commented May 24, 2017

Enhancing ? How ?

well by latching onto the same route and altering / changing the response in some weird way

and why

no idea

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 24, 2017

Member

removed "market".

@tomneedham is checking whether it's really possible to extend/overwrite the endpoints in 10.0. Because from what I remember we removed this possibility a long time ago when unifying the APIs. (something about integrating with app framework I think)

Member

PVince81 commented May 24, 2017

removed "market".

@tomneedham is checking whether it's really possible to extend/overwrite the endpoints in 10.0. Because from what I remember we removed this possibility a long time ago when unifying the APIs. (something about integrating with app framework I think)

@tomneedham

This comment has been minimized.

Show comment
Hide comment
@tomneedham

tomneedham May 24, 2017

Member

Just checked with 10.0.1 => You can still 'extend' existing routes by registering for the same route and returning data - the responses are then 'merged'. This is done using array_merge with priority given to shipped apps. Collisions would only occur when using the same keys in the responses.

Member

tomneedham commented May 24, 2017

Just checked with 10.0.1 => You can still 'extend' existing routes by registering for the same route and returning data - the responses are then 'merged'. This is done using array_merge with priority given to shipped apps. Collisions would only occur when using the same keys in the responses.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 24, 2017

Member

@tomneedham thanks. This kind of weird overriding of routes isn't officially supported. I thought we properly removed this already. Might need more work for this.

This means at this point we will remove the apps anyway from shipped.json. The likeliness of someone overwriting routes like this is anyway minimal.

Waiting for CI then merge

Member

PVince81 commented May 24, 2017

@tomneedham thanks. This kind of weird overriding of routes isn't officially supported. I thought we properly removed this already. Might need more work for this.

This means at this point we will remove the apps anyway from shipped.json. The likeliness of someone overwriting routes like this is anyway minimal.

Waiting for CI then merge

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 24, 2017

Member

@davitol did you test the checkboxes I posted above ? If yes, please tick

Member

PVince81 commented May 24, 2017

@davitol did you test the checkboxes I posted above ? If yes, please tick

@PVince81

This comment has been minimized.

Show comment
Hide comment

@PVince81 PVince81 merged commit 3178356 into master May 24, 2017

3 checks passed

Scrutinizer 1 new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@PVince81 PVince81 deleted the remove-shipped branch May 24, 2017

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