Skip to content
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

Switch configuration to use a config file #21

Open
Eeems opened this issue Nov 27, 2020 · 17 comments
Open

Switch configuration to use a config file #21

Eeems opened this issue Nov 27, 2020 · 17 comments

Comments

@Eeems
Copy link

Eeems commented Nov 27, 2020

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
Copy link
Owner

Evidlo commented Nov 30, 2020

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
Copy link
Author

Eeems commented Nov 30, 2020

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
Copy link

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

@Eeems
Copy link
Author

Eeems commented Dec 8, 2020

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
Copy link
Owner

Evidlo commented Dec 9, 2020

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
Copy link

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.

@Eeems
Copy link
Author

Eeems commented Dec 9, 2020

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
Copy link
Author

Eeems commented Dec 9, 2020

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
Copy link

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
Copy link
Owner

Evidlo commented Dec 17, 2020

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
Copy link
Author

Eeems commented Dec 18, 2020

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
Copy link
Owner

Evidlo commented Dec 18, 2020

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
Copy link
Author

Eeems commented Dec 18, 2020

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
Copy link
Author

Eeems commented Aug 10, 2023

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
Copy link
Owner

Evidlo commented Jan 31, 2024

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
Copy link
Author

Eeems commented Jan 31, 2024

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
Copy link
Owner

Evidlo commented Jan 31, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants