Use, when possible, an ENV var to get the installation type #8500

jredrejo · 2021-10-14T19:13:12Z

Summary

Whenever it's available, use the KOLIBRI_INSTALLATION_TYPE env var to show the installation type instead of trying to guess it

In any case, with or without the env var, the output of the installation type follow the specs at https://docs.google.com/spreadsheets/d/1EpN4yIO9H0qkP0pY6jiEtfQIdyrEPChs3aDZwOMupcI/edit#gid=0

Example:

References

Closes: #7957

Reviewer guidance

Do the new constants names and descriptions make sense?

Testing checklist

Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Critical user journeys are covered by Gherkin stories
Critical and brittle code paths are covered by unit tests

PR process

PR has the correct target branch and milestone
PR has 'needs review' or 'work-in-progress' label
If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
If this is an important user-facing change, PR or related issue has a 'changelog' label
If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

Automated test coverage is satisfactory
PR is fully functional
PR has been tested for accessibility regressions
External dependency files were updated if necessary (yarn and pip)
Documentation is updated
Contributor is in AUTHORS.md

rtibbles · 2021-10-14T19:15:22Z

kolibri/utils/constants/installation_types.py

+WINDOWS = "Windows"
+
+install_type_map = {
+    APK: "APK",


This seems like a good way of constraining the values that end up in our analytics database - @jamalex any concerns with centralizing the possible values here?

It helps with having this centrally documented/enforced, so the various installers don't go and do their own things, yeah.

Main thing is that it'd need to be considered a template/prefix, rather than a simple equivalence match.

indirectlylit

Slightly pedantic but possibly important: these are generally "runtime" types, not "installation" types.

indirectlylit · 2021-10-18T15:43:27Z

kolibri/utils/constants/installation_types.py

+APK = "apk"
+APT = "apt"
+KOLIBRI_SERVER = "kolibri-server"
+MACOS = "Mac"
+PEX = "pex"
+UWSGI = "uwsgi"
+WHL = "whl"
+WINDOWS = "Windows"
+
+install_type_map = {
+    APK: "APK",
+    APT: "APT - Debian package",
+    KOLIBRI_SERVER: "kolibri(apt) with kolibri_server",
+    MACOS: "MacOS Installer",
+    PEX: "PEX file",
+    UWSGI: "UWSGI process",
+    WHL: "WHL Python package",
+    WINDOWS: "Windows Installer",
+}


I'm a bit confused why there are two inconsistent variations of these strings.

Also, if these are permanent strings that cannot be changed we might want to tighten them a bit and make them a bit more consistent and correct:

package manager, file format, or operating system is the primary piece of info?

"apt" is spelled two different ways above

"MacOS" is actually spelled "macOS", and it's technically not an "installer", just an "app"

For "Windows" we might want to distinguish the current "server" version from a potential future "app" version more similar to mac

Suggested change

APK = "apk"

APT = "apt"

KOLIBRI_SERVER = "kolibri-server"

MACOS = "Mac"

PEX = "pex"

UWSGI = "uwsgi"

WHL = "whl"

WINDOWS = "Windows"

install_type_map = {

APK: "APK",

APT: "APT - Debian package",

KOLIBRI_SERVER: "kolibri(apt) with kolibri_server",

MACOS: "MacOS Installer",

PEX: "PEX file",

UWSGI: "UWSGI process",

WHL: "WHL Python package",

WINDOWS: "Windows Installer",

}

ANDROID = "Android APK"

DEBIAN = "Debian package"

DEBIAN_APT = "Debian APT-installed package"

DEBIAN_APT_SERVER = "Debian APT package with kolibri-server"

MACOS_APP = "macOS desktop app"

PEX = "PEX executable"

UWSGI = "UWSGI process"

WHL = "WHL Python package"

WINDOWS_BG = "Windows background process"

runtime_types = (

ANDROID,

DEBIAN,

DEBIAN_APT,

DEBIAN_APT_SERVER,

MACOS_APP,

PEX,

UWSGI,

WHL,

WINDOWS_BG,

)

The spec for this is here: #7960 this does need some updates to fit that spec (such as being able to insert the version number), but better to iterate there. Also, as there is already historic data in this format for some installers, updates may not be preferred for consistent analytics reporting.

Ah, didn't realize there there was already data. At the least then there should only be one set of strings, not two? And we can still choose more consistent constant names even if the actual strings are a bit noisy.

@rtibbles great, I was not aware of #7960 happy to fix these inconsistences after knowing the details.

@indirectlylit I agree with some of your changes (I didn't know we have an app for MacOs instead of an installer) but not with others, For Windows, for example installation type is clearly an installer, not a background process.
Also , there's not practical way to know if a Debian package has been installed using dpkg or apt, the current logic tries to find it out, but it can be easily cheated. In my opinion, we should just say "Deb package" removing the apt reference.

for what it's worth these comments are not critical or intended to be blocking.

For Windows, for example installation type is clearly an installer, not a background process.

Is it the "installation type" we care about or the runtime app? Sometimes it's both but often they are independent. This is like the Debian package which can be installed either with dpkg or apt.

The APK can be installed by sideloading or via the app store.

The current Windows repo contains three things:

an installer script which runs once

a background process (kolibri.exe) which runs Kolibri in the background, potentially on start-up

a small GUI which allows some basic config and control

Additionally at some point we'll also have a Windows "foreground app" similar to the Endless, Android, and macOS apps based on this repo. When this happens, it may also have its own installer.

In my opinion, we should just say "Deb package" removing the apt reference.

agreed!

Is it the "installation type" we care about or the runtime app?

well, this is a part of a function called installation_type 😄 , probably knowing the runtime app is useful too, but when trying to get information from the user to solve issues, knowing how Kolibri was installed is important.

jamalex · 2021-11-17T19:12:45Z

Main thing here is updating to match the spec:
#7960 (comment)

(and, as noted in the comment, ensuring that the set of constants are treated as prefixes, rather than exact matches)

jredrejo · 2021-11-18T19:55:15Z

@jamalex done according to the specs (I have updated the description of this PR accordingly).
Now it's on the respective installers to set the env var if we want to be exact, if not , the installation_type function tries to guess it. Issues for this are linked in #7957
For the version, I know how to get the version of the apk, deb and kolibri-server installers but not for the rest, so by now, the version is not returned.

jamalex

This looks good to me overall, functionally!

I note that it seems the install_type variable is used to hold the keys from the constants, rather than the values, but we also store the value from the envvar in there. I had been imagining that the envvar passed in from the installer would be the full value (including the version, etc). Do you see any issues with that, either on the installer side (detecting version etc there, to put in the envvar) or here, with the codepaths (it seems like it would probably just skip over the other code and directly return the value from the envvar, so we'd be fine here)? It's nice having the fallback logic here just in case, too.

jredrejo · 2021-11-19T17:37:25Z

This looks good to me overall, functionally!

I note that it seems the install_type variable is used to hold the keys from the constants, rather than the values, but we also store the value from the envvar in there. I had been imagining that the envvar passed in from the installer would be the full value (including the version, etc). Do you see any issues with that, either on the installer side (detecting version etc there, to put in the envvar) or here, with the codepaths (it seems like it would probably just skip over the other code and directly return the value from the envvar, so we'd be fine here)? It's nice having the fallback logic here just in case, too.

My main concern is having envvars with spaces or other chars to separate version and installer type, I am not sure that will work correctly in the different combinations of OS/installers we have. If you prefer the installer to provide the version I'd go for one envvar for the installer type and another for the version.
I have added a KOLIBRI_INSTALLER_VERSION envvar that, if set by the installer, will be used by kolibri instead of guessing it out.

indirectlylit · 2021-11-23T16:00:59Z

kolibri/utils/server.py

+            if hasattr(output, "decode"):  # needed in python 2.x
+                output = output.decode("utf-8")
+            package_info = output.split("\n")
+            version_info = [output for output in package_info if "Version" in output]


Not sure why this line didn't raise an issue before, but it's now causing a python 2 syntax check flake8 failure:

kolibri/utils/server.py:929:40: F812 list comprehension redefines 'output' from line 927

https://github.com/indirectlylit/kolibri/runs/4301449205?check_suite_focus=true

Perhaps this should be:

version_info = [info for info in package_info if "Version" in info]

Weirdly, I see the linting error in my fork but not in the LE org repo.

cc @jredrejo @rtibbles

interesting, the check is skipped in LE but not in my repo:

https://github.com/learningequality/kolibri/runs/4301448839?check_suite_focus=true

Oh - probably because the path match check looks back through the history of runs (which are constrained to a specific fork) so because the learning equality repo had a test run that had previously passed for this set of files, it doesn't run again, whereas your fork didn't.

filed a follow-up here: #8772

weird, that's not new code, it has been there since kolibri 0.11

I think it is more likely a floating linter version issue than an issue with the CI setup.

jredrejo added the TODO: needs review Waiting for review label Oct 14, 2021

jredrejo added this to the 0.15.0 milestone Oct 14, 2021

jredrejo requested a review from rtibbles October 14, 2021 19:13

rtibbles reviewed Oct 14, 2021

View reviewed changes

indirectlylit reviewed Oct 18, 2021

View reviewed changes

jamalex self-requested a review October 28, 2021 17:25

José Redrejo added 4 commits November 18, 2021 18:52

Use, when possible, an ENV var to get the installation type

82d8c92

add uwsgi

7b022ba

add forgotten file to git

8ab172c

fine tunning app installation method names

2c8d77d

jredrejo force-pushed the installation_type branch from 73d4185 to 2c8d77d Compare November 18, 2021 17:53

Match specs to installation types for telemetry

aba4442

jamalex approved these changes Nov 19, 2021

View reviewed changes

fix test & add KOLIBRI_INSTALLER_VERSION envvar

1b19e20

jredrejo merged commit f303529 into learningequality:release-v0.15.x Nov 19, 2021

jredrejo deleted the installation_type branch November 19, 2021 19:22

indirectlylit reviewed Nov 23, 2021

View reviewed changes

indirectlylit mentioned this pull request Nov 23, 2021

intermittent lint failure #8772

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use, when possible, an ENV var to get the installation type #8500

Use, when possible, an ENV var to get the installation type #8500

jredrejo commented Oct 14, 2021 •

edited

rtibbles Oct 14, 2021

jamalex Nov 1, 2021

indirectlylit left a comment

indirectlylit Oct 18, 2021 •

edited

rtibbles Oct 18, 2021

indirectlylit Oct 18, 2021

jredrejo Oct 18, 2021 •

edited

indirectlylit Oct 18, 2021 •

edited

jredrejo Oct 18, 2021

jamalex commented Nov 17, 2021

jredrejo commented Nov 18, 2021

jamalex left a comment •

edited

jredrejo commented Nov 19, 2021 •

edited

indirectlylit Nov 23, 2021

indirectlylit Nov 23, 2021 •

edited

rtibbles Nov 23, 2021

indirectlylit Nov 23, 2021

jredrejo Nov 23, 2021 •

edited

rtibbles Nov 23, 2021

Use, when possible, an ENV var to get the installation type #8500

Use, when possible, an ENV var to get the installation type #8500

Conversation

jredrejo commented Oct 14, 2021 • edited

Summary

References

Reviewer guidance

Testing checklist

PR process

Reviewer checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indirectlylit left a comment

Choose a reason for hiding this comment

indirectlylit Oct 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jredrejo Oct 18, 2021 • edited

Choose a reason for hiding this comment

indirectlylit Oct 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamalex commented Nov 17, 2021

jredrejo commented Nov 18, 2021

jamalex left a comment • edited

Choose a reason for hiding this comment

jredrejo commented Nov 19, 2021 • edited

Choose a reason for hiding this comment

indirectlylit Nov 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jredrejo Nov 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jredrejo commented Oct 14, 2021 •

edited

indirectlylit Oct 18, 2021 •

edited

jredrejo Oct 18, 2021 •

edited

indirectlylit Oct 18, 2021 •

edited

jamalex left a comment •

edited

jredrejo commented Nov 19, 2021 •

edited

indirectlylit Nov 23, 2021 •

edited

jredrejo Nov 23, 2021 •

edited