Skip to content

Added apps updates monitoring - #99

Merged
MorrisJobke merged 7 commits into
nextcloud:masterfrom
patschi:add-apps-monitoring
Aug 30, 2018
Merged

Added apps updates monitoring#99
MorrisJobke merged 7 commits into
nextcloud:masterfrom
patschi:add-apps-monitoring

Conversation

@patschi

@patschipatschi commented Jun 12, 2017

Copy link
Copy Markdown
Member

Adds apps updates monitoring to API endpoint, as requested in #86

@patschi
Added apps updates monitoring,closes#86
8714f21
Signed-off-by: Patrik Kernstock <info@pkern.at>
@patschi
Add enabled apps for monitoring endpoint as well
3d85e15
Signed-off-by: Patrik Kernstock <info@pkern.at>
MorrisJobke
MorrisJobke reviewed Jul 5, 2017
View reviewed changes
Comment threadlib/SystemStatistics.php Outdated
];

// load all apps
$apps = (new OC_App())->listAllApps();

@MorrisJobkeMorrisJobkeJul 5, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you use \OCP\IAppManager::getInstalledApps() instead? (https://github.com/nextcloud/server/blob/591e75df5c3acf51e6968f20b1856481ee56f4de/lib/public/App/IAppManager.php#L116-L116) It's a list of app ids. And then also the CI job succeeds, because you don't use private namespace anymore ;)

MorrisJobke
MorrisJobke requested changes Jul 5, 2017
View reviewed changes

@MorrisJobkeMorrisJobke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See inline comment

@patschi
Rework: Retrieve installed apps using IAppManager
13ef75a
Removing 'num_enabled' key temporary as the new IAppManager-way seems not to provide any reliable way to tell all activated (or even the count) apps. May follow later.
Signed-off-by: Patrik Kernstock <info@pkern.at>
@patschi

patschi commented Jul 5, 2017

Copy link
Copy Markdown
MemberAuthor

Thanks a lot for your feedback! Was searching quite a while for the right way to get out all installed apps... couldn't find any. As I found the exact way, before my recent commit, in an other app, I've just used the same way.

However, builds seems passing. Actually I got into one new issue that I was not able to find out the correct way to get all activated apps at all. Using IAppManager I just found the following and closest ways:

  • getAlwaysEnabledApps(), which are forced-online apps which are required like files app
  • and getEnabledAppsForUser(IUser $user) for getting activated apps depending on the user. But the monitoring endpoint is userless?

So I have just removed num_enabled count for now. Either we'll find a better way and implement that, or we just add it someday later back.

@MorrisJobke

MorrisJobke commented Jul 6, 2017

Copy link
Copy Markdown
Member

So I have just removed num_enabled count for now. Either we'll find a better way and implement that, or we just add it someday later back.

Okay for me for now 👍

@tflidd

tflidd commented Jun 6, 2018

Copy link
Copy Markdown
Contributor

What is the status of this PR? There are already conflicts preventing to merge this. Would be great to know if this is a wanted change that we can resolve the conflicts and merge it in. It's open for nearly a year.

@MorrisJobke

MorrisJobke commented Jun 7, 2018

Copy link
Copy Markdown
Member

Makes sense to add it IMO 👍

cc @blizzz@rullzer

@rullzer

rullzer commented Jun 7, 2018

Copy link
Copy Markdown
Member

There is a conflict.
But in general sure lets do this!

@patschi

patschi commented Jul 18, 2018

Copy link
Copy Markdown
MemberAuthor

Conflict resolved.
@MorrisJobke@rullzer

Probably also worth checking other PRs, like #98 and #97?

@MorrisJobkeMorrisJobke added this to the Nextcloud 14 milestone Aug 20, 2018
nickvergessen
nickvergessen reviewed Aug 23, 2018
View reviewed changes
Comment threadlib/SystemStatistics.php Outdated

/** @var View view on data/ */
private $view;
/** @var appFetcher */

@nickvergessennickvergessenAug 23, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

AppFetcher

nickvergessen
nickvergessen requested changes Aug 23, 2018
View reviewed changes
Comment threadlib/SystemStatistics.php Outdated
private $view;
/** @var appFetcher */
private $appFetcher;
/** @var appManager */

@nickvergessennickvergessenAug 23, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IAppManager

Comment threadlib/SystemStatistics.php
* @param IConfig $config
* @param IConfig $config
* @param IAppManager $appManager
*/

@nickvergessennickvergessenAug 23, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

remove the doc block, just duplicating the method signature anyway.
Or you add the $appFetcher

@patschi
Some phpdoc fixes
817bc41
Signed-off-by: Patrik Kernstock <info@pkern.at>
@patschi

patschi commented Aug 23, 2018

Copy link
Copy Markdown
MemberAuthor

@nickvergessen Applied changes as requested. I've decided to just add the $appFetcher line within the constructor.

@tflidd

tflidd commented Aug 27, 2018

Copy link
Copy Markdown
Contributor

You probably want to merge this branch into master (not the other way round like your did in 13e5bd2

@MorrisJobkeMorrisJobke mentioned this pull request Aug 30, 2018
Merged
6 tasks
@MorrisJobke
MorrisJobke merged commit 1e20926 into nextcloud:masterAug 30, 2018
@joshtrichardsjoshtrichards mentioned this pull request Nov 21, 2023
Open
4 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessennickvergessennickvergessen approved these changes
@MorrisJobkeMorrisJobkeMorrisJobke approved these changes
@IvansssIvansssAwaiting requested review from Ivansss

Assignees

No one assigned

Labels

3. to reviewenhancement

Projects

None yet

Milestone

Nextcloud 14

Development

Successfully merging this pull request may close these issues.

5 participants

@patschi@MorrisJobke@tflidd@rullzer@nickvergessen