Evidlo / remarkable_news

Daily news/comics on your reMarkable's suspend screen
GNU General Public License v3.0
285 stars 32 forks source link

Switch configuration to use a config file #21

Open Eeems opened 4 years ago

Eeems commented 4 years ago

Currently the service file is modified at install time with information like timezone and cooldown settings. These should be moved to a configuration file. This will allow simplifying the install step, as well as make it easier for this to be packaged up in toltec.

Timezone should also be pulled from the device itself instead of passed in. You can easily do this by parsing the output of timedatectl status.

Evidlo commented 4 years ago

Assuming the variables in the service files are removed, couldn't the toltec package simply install renews and all the available services, then let the user decide which one to enable with systemctl?

Eeems commented 4 years ago

That is also an option, but in order to do it properly the service files will need to be updated with proper conflicts in place.

owulveryck commented 3 years ago

how about switching to environment variables instead of a configuration file?

Eeems commented 3 years ago

how about switching to environment variables instead of a configuration file?

And where would you store these environment variables? I'd rather not have to ask users to modify the service file.

Evidlo commented 3 years ago

I just eliminated the -timezone argument. For the other variables, why not just replace them with sed in the toltec build script just like the Makefile does?

build() {
    make renews.arm
    sed -i "s|COOLDOWN|3600|" services/*.service
    sed -i "s|KEYWORDS|scenery,buildings|" services/loremflickr.service
}
owulveryck commented 3 years ago

how about switching to environment variables instead of a configuration file?

And where would you store these environment variables? I'd rather not have to ask users to modify the service file.

The environment variables can be stocked anywhere in a .envrc file. This removes extra check inside the application:

with a package such as https://github.com/sethvargo/go-envconfig you can even deal with complex types (I am thinking about the cooldown period which is a duration), default variables, and required parameters easily.

This would also be helpful for the "test" option where you could store the configs of the providers in an .envrc in a peculiar directory and use direnv. ex:

so when you are entering the directory the env files are loaded automatically and you can simple run the utility.

Eeems commented 3 years ago

I just eliminated the -timezone argument. For the other variables, why not just replace them with sed in the toltec build script just like the Makefile does?

build() {
    make renews.arm
    sed -i "s|COOLDOWN|3600|" services/*.service
    sed -i "s|KEYWORDS|scenery,buildings|" services/loremflickr.service
}

It's an option, but that would mean that we'd have to publish multiple packages that are basically the same thing, and handle the conflicts between them. I'm open to it as an option, but I think it makes more sense to make it user configurable after install, so that the package manager doesn't need to be used to install specific configurations.

Eeems commented 3 years ago

how about switching to environment variables instead of a configuration file?

And where would you store these environment variables? I'd rather not have to ask users to modify the service file.

The environment variables can be stocked anywhere in a .envrc file. This removes extra check inside the application:

* no need to check if the file exists

* if it is accessible

* if it is readable

* if the format is as expected

* ...

with a package such as https://github.com/sethvargo/go-envconfig you can even deal with complex types (I am thinking about the cooldown period which is a duration), default variables, and required parameters easily.

This would also be helpful for the "test" option where you could store the configs of the providers in an .envrc in a peculiar directory and use direnv. ex:

* xkcd/.envrc

* nyt/.envrc

so when you are entering the directory the env files are loaded automatically and you can simple run the utility.

Sounds like a complex way to just have a config file that doesn't really have benefits for the use case I'm trying to steward towards (installation from a package manager). It seems more useful for someone doing initial development, or testing of the application. Most users will install, set up config once, and then never want to touch it again.

owulveryck commented 3 years ago

I tested it; basically. Here is the result of the test:

the conf is stored in /home/root/renews/xkcd.envrc:

NEWFECTHER_GENERIC_URL="https://xkcd.com"
NEWFECTHER_GENERIC_XPATH="//div[@id='comic']/img/@src"
NEWFECTHER_GENERIC_MODE="center"
NEWFECTHER_GENERIC_SCALE=1.75
RKNEWS_UPDATE_FREQUENCY="1h"

Then in systemd I reference the file

[Unit]
Description=XKCD

[Service]
ExecStart=/home/root/renews/renews.arm 
EnvironmentFile=/home/root/renews/xkcd.envrc
Restart=always

[Install]
WantedBy=multi-user.target

The parsing is done per provider with default values:

// Provider provider
type Provider struct {
    LivenessCheck    time.Duration `env:"LIVENESS_CHECK,default=5m"`
    ProbeTimeout     time.Duration `env:"PROBE_TIMEOUT,default=60m"`
    HTTPTimeout      time.Duration `env:"HTTP_TIMEOUT,default=10s"`
    TransportTimeout time.Duration `env:"TRANSPORT_TIMEOUT,default=5s"`
    URL              *url.URL      `env:"URL,required"`
    Path             string        `env:"XPATH"`
    Mode             string        `env:"MODE,default=center"` // fill is possible
    Scale            float64       `env:"SCALE, default=1"`
    lookuper         envconfig.Lookuper // This is a commodity for implementing _tests
    httpClient       *http.Client 
}

With sethvargo's envconfig package which takes cares of required configuration as well as default values:

    var l envconfig.Lookuper
    l = c.lookuper
    if c.lookuper == nil {
        l = envconfig.PrefixLookuper("NEWFECTHER_GENERIC_", envconfig.OsLookuper())
    }
    err := envconfig.ProcessWith(ctx, c, l)
    if err != nil {
        return err
    }

Note that we can directly use time duration with the configuration.

Amongst over advantages (no need to implement file reading in the code, easy way to run it out-of-systemd, and so on), this implementation makes it possible to implement a configuration per provider. Imagine you want a specific provider instead of relying on the generic one such as natgeo that we find in the code.

Evidlo commented 3 years ago

It's an option, but that would mean that we'd have to publish multiple packages that are basically the same thing, and handle the conflicts between them.

Why? Just publish the one package with all the services with default values. The user will install renews and then enable the service they want.

Eeems commented 3 years ago

It's an option, but that would mean that we'd have to publish multiple packages that are basically the same thing, and handle the conflicts between them.

Why? Just publish the one package with all the services with default values. The user will install renews and then enable the service they want.

Which is not what your install currently does, currently you install one service based on what they want to use.

Evidlo commented 3 years ago

The toltec build file wouldn't use my Makefile for installing the service. Here is a sample toltec package which I have not tested

#!/usr/bin/env bash
# Copyright (c) 2020 The Toltec Contributors
# SPDX-License-Identifier: MIT

pkgnames=(remarkable-news)
...
sha256sums=(xxxx)

build() {
    make renews.arm
    sed -i "s|COOLDOWN|3600|" services/*.service
    sed -i "s|KEYWORDS|scenery,buildings|" services/loremflickr.service
}

package() {
    install -D -m 644 "$srcdir"/services/* "$pkgdir"/etc/systemd/system
}

configure() {
    systemctl daemon-reload
    if ! is-enabled "$pkgname.service"; then
        echo ""
        echo "Choose your suspend screen by enabling one of these services"
        for service in services/*; do
            how-to-enable "$service"
        done
        echo ""
    fi
}
Eeems commented 3 years ago

So this brings me back to an earlier statement, they will all need to have a proper CONFLICTS specified so that the user can't enable two of them at the same time.

Eeems commented 1 year ago

Another concern about adding multiple service files is that the root partition is always low on space, so we would also like to avoid putting stuff in the root partition as much as possible.

Another option we could have would be to use the environment files, stored in /opt/etc/re_news and use a template systemd unit, so that you can pick which one you want to enable with systemctl enable --now renews@xkcd

Evidlo commented 10 months ago

I think I'd prefer to have a separate toltec package for each service which all depend on a main remarkable-news package that contains the actual binary.

Can replaces/provides be used in some way to create a virtual remarkable-news-service package that all the service packages provide that cause them to conflict with each other? It would be nice not to have to update conflicts for every service package any time a new service is added.

I'm trying to keep configuration as packages so that a user can use the nao package manager for selecting the service.

Eeems commented 10 months ago

I think I'd prefer to have a separate toltec package for each service which all depend on a main remarkable-news package that contains the actual binary.

Can replaces/provides be used in some way to create a virtual remarkable-news-service package that all the service packages provide that cause them to conflict with each other? It would be nice not to have to update conflicts for every service package any time a new service is added.

I'm trying to keep configuration as packages so that a user can use the nao package manager for selecting the service.

From the testing I've done with provides and replaces, it doesn't appear to work with opkg the same way it does for other package managers. I've had to work around it for now with splashscreens by just having them all provide the same file, so it will refuse to install multiple packages with the same file.

Evidlo commented 10 months ago

OK, then it seems like making each service package provide renews.service would solve the issue. The COOLDOWN is now gone, and KEYWORDS can just default to something generic like nature.