New module: app_info #259

aalaesar · 2023-04-21T17:09:17Z

Add a simple module to get application informations (status, update, etc)
This is intended to be a foundation for the next module for app management

staticdev · 2023-04-21T17:18:35Z

@aalaesar package managers in general resolve better if you send the complete list of apps in one command. I know this works for example for apt and pip packages. Not sure if it is the case for occ, but could be worth to try for make it more efficient.

Btw @wiktor2200 met me in person today by chance and recognized me xD

aalaesar · 2023-04-23T21:21:33Z

@aalaesar package managers in general resolve better if you send the complete list of apps in one command. I know this works for example for apt and pip packages. Not sure if it is the case for occ, but could be worth to try for make it more efficient.

It is not the case the the occ cli tool unfortunately, there are options to get all info at once but it make it less actionable.
The current issue is that , for gathering infos about all applications on a moderate instance, the module take just too much time...
hmm
I suspected that calling occ would be costly in time (maybe that's how the run_occ function is implemented that is not optimized)
There is indeed the risk for the module to badly scale (in running time) in case of large amount of application installed on an instances.
In that case I think splitting the information gathering In Two modules would be simplier:

one module to get facts about the nexcloud server with general information (all applications and update available), easy to get in a static and limited number of calls to occ.
one app_info module to get more details about one application in particular

Signed-off-by: Marc Crebassa <[email protected]>

aalaesar · 2023-07-30T15:56:49Z

Hello there.
I'm back after some personal issues that took me away from my projects.
I wanted to finish the app_info module.
From my point of vue, the module is finished
but the CI seems broken with concurrency issues with docker and the tests running in parallel, and also the occ install command failing somehow 😕
as the change is not related to the installation role, this can be merged.

aalaesar · 2023-08-22T14:57:18Z

@staticdev @wiktor2200
Gentle up 😄 could you review the module?
it's a fairly simple one as it only gather infos for only one app

wiktor2200

Hi! @aalaesar sorry for late review.
I've analyzed your code and it looks good.
I'm not convinced only by naming for "shipped" apps, I'd personally rename it to "pre-installed", "default", "enabled_by_default' or similar.
Here in documentation https://docs.nextcloud.com/server/latest/admin_manual/apps_management.html#apps-management they call them: default enabled or enabled by default.

But it's up to you, so I leave approve for your code :)

staticdev

Sorry also for taking long and welcome back! From my side the functional side looks good. I found a couple of issues with the imports, some are super minor/cosmetic but anyway they should be on top level.

staticdev · 2023-08-23T04:56:31Z

plugins/modules/app_info.py

@@ -0,0 +1,194 @@
+#!/usr/bin/python
+# -*- coding: utf-8 -*-


Suggested change

# -*- coding: utf-8 -*-

import copy

import json

from ansible.module_utils.basic import AnsibleModule

from ansible_collections.nextcloud.admin.plugins.module_utils.occ import \

run_occ

from ansible_collections.nextcloud.admin.plugins.module_utils.occ_args_common import \

OCC_ARGS_SPEC

Encoding is not necessary to add anymore uft-8 is the default https://stackoverflow.com/questions/14083111/should-i-use-encoding-declaration-in-python-3

Instead I would add all imports here, see explanation below.

staticdev · 2023-08-23T04:57:16Z

plugins/modules/app_info.py

+#!/usr/bin/python
+# -*- coding: utf-8 -*-
+
+# Copyright: (c) 2023, Marc Crébassa <[email protected]>


I would also avoid adding license to each file since we already have clear license settings on Github and on the repo.

staticdev · 2023-08-23T05:01:24Z

plugins/modules/app_info.py

+
+import copy
+import json
+from ansible.module_utils.basic import AnsibleModule


It is normally good to separate build-in imports from 3rd party

import json from ansible.module_utils.basic import AnsibleModule

staticdev · 2023-08-23T05:02:57Z

plugins/modules/app_info.py

+from ansible_collections.nextcloud.admin.plugins.module_utils.occ import run_occ
+from ansible_collections.nextcloud.admin.plugins.module_utils.occ_args_common import (
+ OCC_ARGS_SPEC,
+)


I would do long imports like this:

from ansible_collections.nextcloud.admin.plugins.module_utils.occ import \ run_occ from ansible_collections.nextcloud.admin.plugins.module_utils.occ_args_common import \ OCC_ARGS_SPEC

I think that part was formatted by only-black

staticdev · 2023-08-23T05:05:21Z

plugins/modules/app_info.py

+ returned: success
+"""
+
+import copy


Imports should come right after #!/usr/bin/python, in this case is not a good idea to declare variables before imports.

aalaesar · 2023-08-23T06:53:10Z

I'm not convinced only by naming for "shipped" apps, I'd personally rename it to "pre-installed", "default", "enabled_by_default' or similar. Here in documentation https://docs.nextcloud.com/server/latest/admin_manual/apps_management.html#apps-management they call them: default enabled or enabled by default.

But it's up to you, so I leave approve for your code :)

Hello there ! 😃
Thank you for your feedback .
Regarding your 2 points:
I'm simply reusing the terminology used by the occ cli to list or filter the apps that comes preinstalled with the server, but I'm not saying it's the best word.
Regarding pre installed apps I think they are not all pre-enabled, so its hard to tell which one was enabled at any given time, as the list of enabled apps is live, and there is no filter of my knowledge to list de pre enabled apps, I can't generate this info.

aalaesar · 2023-08-23T07:05:59Z

Sorry also for taking long and welcome back! From my side the functional side looks good. I found a couple of issues with the imports, some are super minor/cosmetic but anyway they should be on top level.

Hello There ! 😃
Thank you for your feedback.
Regarding your comments we will have to decide between official ansible doc regarding module code format or python formatting good practices 😕
Regards
Aal.

wiktor2200 · 2023-08-23T08:23:21Z

I'm simply reusing the terminology used by the occ cli to list or filter the apps that comes preinstalled with the server, but I'm not saying it's the best word.
Regarding pre installed apps I think they are not all pre-enabled, so its hard to tell which one was enabled at any given time, as the list of enabled apps is live, and there is no filter of my knowledge to list de pre enabled apps, I can't generate this info.

Ok, so let's stick to one naming convention or we would to change it everywhere otherwise (we still can in the future).
And that's true, pre-enabled apps are hard to determine and I think it also has changed over time in official Nextcloud repo, so I agree with current solution. :)

staticdev · 2023-08-27T22:33:22Z

Sorry also for taking long and welcome back! From my side the functional side looks good. I found a couple of issues with the imports, some are super minor/cosmetic but anyway they should be on top level.

Hello There ! 😃 Thank you for your feedback. Regarding your comments we will have to decide between official ansible doc regarding module code format or python formatting good practices 😕 Regards Aal.

Oh my, this documentation really surprises me. Looks like it is stopped in time. In this case, let's keep your solution. If I have some time i will create an issue on Ansible side.

aalaesar force-pushed the module/app_info branch from 7031f07 to dd1aa23 Compare April 21, 2023 17:15

wiktor2200 added enhancement New feature or request python Pull requests that update Python code labels Apr 27, 2023

aalaesar added 3 commits July 30, 2023 14:01

First draft of app_infos

dc6d21b

Signed-off-by: Marc Crebassa <[email protected]>

fix missing requirement on common parameter

7574de6

Signed-off-by: Marc Crebassa <[email protected]>

simplify app_info to get info for one app only

ab514ab

Signed-off-by: Marc Crebassa <[email protected]>

aalaesar force-pushed the module/app_info branch from 364052b to ab514ab Compare July 30, 2023 12:01

fix contact version used for tests

9a185f4

Signed-off-by: Marc Crebassa <[email protected]>

aalaesar marked this pull request as ready for review July 30, 2023 15:50

aalaesar requested review from staticdev and wiktor2200 July 30, 2023 15:51

aalaesar changed the title ~~first draft of app_infos~~ New module: app_info Aug 22, 2023

wiktor2200 approved these changes Aug 22, 2023

View reviewed changes

staticdev requested changes Aug 23, 2023

View reviewed changes

staticdev approved these changes Aug 27, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New module: app_info #259

New module: app_info #259

aalaesar commented Apr 21, 2023 •

edited

staticdev commented Apr 21, 2023 •

edited

aalaesar commented Apr 23, 2023 •

edited

aalaesar commented Jul 30, 2023

aalaesar commented Aug 22, 2023

wiktor2200 left a comment

staticdev left a comment

staticdev Aug 23, 2023

staticdev Aug 23, 2023

staticdev Aug 23, 2023

staticdev Aug 23, 2023

aalaesar Aug 28, 2023

staticdev Aug 23, 2023

aalaesar commented Aug 23, 2023

aalaesar commented Aug 23, 2023

wiktor2200 commented Aug 23, 2023

staticdev commented Aug 27, 2023

New module: app_info #259

Are you sure you want to change the base?

New module: app_info #259

Conversation

aalaesar commented Apr 21, 2023 • edited

staticdev commented Apr 21, 2023 • edited

aalaesar commented Apr 23, 2023 • edited

aalaesar commented Jul 30, 2023

aalaesar commented Aug 22, 2023

wiktor2200 left a comment

Choose a reason for hiding this comment

staticdev left a comment

Choose a reason for hiding this comment

staticdev Aug 23, 2023

Choose a reason for hiding this comment

staticdev Aug 23, 2023

Choose a reason for hiding this comment

staticdev Aug 23, 2023

Choose a reason for hiding this comment

staticdev Aug 23, 2023

Choose a reason for hiding this comment

aalaesar Aug 28, 2023

Choose a reason for hiding this comment

staticdev Aug 23, 2023

Choose a reason for hiding this comment

aalaesar commented Aug 23, 2023

aalaesar commented Aug 23, 2023

wiktor2200 commented Aug 23, 2023

staticdev commented Aug 27, 2023

aalaesar commented Apr 21, 2023 •

edited

staticdev commented Apr 21, 2023 •

edited

aalaesar commented Apr 23, 2023 •

edited