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

feat(config): make it so you can revert override of apps #440

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

StealthyCoder
Copy link
Member

This is to add a way to disable the override, inherit from parent concerning apps running on devices.

Relevant fioconfig code

Comment on lines 239 to 244
err = sota.Delete("pacman.docker_apps")
if err != nil {
fmt.Printf("Error trying to delete pacman.docker_apps key: %s\n", err)
}
sota.Delete("pacman.compose_apps")
if err != nil {
fmt.Printf("Error trying to delete pacman.compose_apps key: %s\n", err)
}
Copy link
Member

@vkhoroz vkhoroz Nov 4, 2024

Choose a reason for hiding this comment

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

Looking at sota.Delete IIUC there are two use cases when it can return an error:

  1. A key is malformed.
  2. A key is not found.

Here, (1) is a programming error deserving DieNotNil, while (2) would be better off silently ignored.
Thus, I suggest to improve this code using sota.Has.

Suggested change
err = sota.Delete("pacman.docker_apps")
if err != nil {
fmt.Printf("Error trying to delete pacman.docker_apps key: %s\n", err)
}
sota.Delete("pacman.compose_apps")
if err != nil {
fmt.Printf("Error trying to delete pacman.compose_apps key: %s\n", err)
}
if sota.Has("pacman.docker_apps") {
subcommands.DieNotNil(sota.Delete("pacman.docker_apps"))
}
if sota.Has("pacman.compose_apps") {
subcommands.DieNotNil(sota.Delete("pacman.compose_apps"))
}

Copy link
Member

Choose a reason for hiding this comment

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

The other option, as we know a key we pass is properly formatted, would be to make an "optimistic" assumption that sota.Delete always succeeds from a practical perspective:

Suggested change
err = sota.Delete("pacman.docker_apps")
if err != nil {
fmt.Printf("Error trying to delete pacman.docker_apps key: %s\n", err)
}
sota.Delete("pacman.compose_apps")
if err != nil {
fmt.Printf("Error trying to delete pacman.compose_apps key: %s\n", err)
}
_ = sota.Delete("packman.docker_apps")
_ = sota.Delete("packman.compose_apps")

Copy link
Member Author

Choose a reason for hiding this comment

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

Even though we have properly formatted ones, maybe in the future some other error check will be introduced so I went with the aforementioned one. Check of the key and delete it.

fioctl config updates --group beta --apps shellhttpd --tag master`,
fioctl config updates --group beta --apps shellhttpd --tag master

# Set the docker apps to none for devices:
Copy link
Member

Choose a reason for hiding this comment

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

explain that none means "run no apps"

# Set the docker apps to none for devices:
fioctl config updates --apps ,

# Set the docker apps to inherit from parent for devices:
Copy link
Member

Choose a reason for hiding this comment

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

explain what "parent" means.

fioctl devices config updates <device> --apps ,

# Set the docker apps to inherit from parent:
fioctl devices config updates <device> --apps -`,
Copy link
Member

Choose a reason for hiding this comment

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

explain what "parent" and "none" mean here also

Copy link
Member

Choose a reason for hiding this comment

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

I would state explicitly here that we are inheriting from the group config.

Suggested change
fioctl devices config updates <device> --apps -`,
# Set the docker apps to inherit from a group config:

We might be more verbose (like below), but I think that is apparent from the fioctl config updates help.

Suggested change
fioctl devices config updates <device> --apps -`,
# Set the docker apps to inherit from a group config (if set there) or a system default (otherwise):

Copy link
Member

Choose a reason for hiding this comment

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

Once I wrote this I understood we don't yet have a way to restore the device tag to inherit from a group config or system default.
Do we need one?

@doanac @mike-scott ^^^

Copy link
Member

Choose a reason for hiding this comment

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

probably should do it also

@mike-sul
Copy link
Contributor

mike-sul commented Nov 5, 2024

Tangential note: we don't need to support pacman.docker_apps not sure we've ever used it.
We deprecated it in v75 in this commit foundriesio/aktualizr-lite@34f8f70

# Set the docker apps to inherit from their parent, meaning:
# If for a device apps are not set - all apps are installed
  # If for a device apps are set - those apps are installed
Copy link
Member

Choose a reason for hiding this comment

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

We still aren't explain what the "parent" will be. I'm not sure parent is a good word. What we are trying to describe is how are config files are processed. We use the same logic as systemd where we walk through:

  • /usr/lib/sota/conf.d/
  • /var/sota/sota.toml
  • /etc/sota/conf.d/

We need a concise way to explain this.

And that , and - are so subtle here that we should probably call out that they are special characters.

Copy link
Member

Choose a reason for hiding this comment

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

IIUC the group level is the top one here (as we allow no factory-wide updates config).
So, probably, we may put here a sentence like this:

Suggested change
  # If for a device apps are set - those apps are installed
# Set the docker apps to a system default (all apps on most devices):

A system default seems to be rather specific to me, without diving into internal details (which can differ from device to device).

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

Successfully merging this pull request may close these issues.

4 participants