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

subcommands/config/updates.go Outdated Show resolved Hide resolved
subcommands/config/updates.go Outdated Show resolved Hide resolved
subcommands/devices/config_updates.go Outdated Show resolved Hide resolved
@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

@StealthyCoder StealthyCoder force-pushed the feat/restore_apps_override branch 2 times, most recently from 7d7142f to 854ec25 Compare November 5, 2024 10:22
Copy link
Member

@doanac doanac left a comment

Choose a reason for hiding this comment

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

technically it looks good. Lets see what @kprosise says about the wording

@doanac doanac requested a review from kprosise November 6, 2024 17:21
Copy link
Contributor

@kprosise kprosise left a comment

Choose a reason for hiding this comment

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

LGTM, the latest changes seem to address feedback provided by other reviewers. Only suggestion has to do with style, but imho the descriptions communicate the usage well, neither too verbose or terse.

subcommands/config/updates.go Outdated Show resolved Hide resolved
subcommands/config/updates.go Outdated Show resolved Hide resolved
subcommands/devices/config_updates.go Outdated Show resolved Hide resolved
subcommands/devices/config_updates.go Outdated Show resolved Hide resolved
@mike-sul
Copy link
Contributor

mike-sul commented Nov 7, 2024

My 2 cents on the terms we use. IMHO, we should use the term "Compose apps" not "Docker apps" to make it consistent with the user doc.

Copy link
Member

@vkhoroz vkhoroz left a comment

Choose a reason for hiding this comment

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

LGTM

Once Katrina is happy, this can be merged.

Comment on lines +29 to +47
# There are two special characters: "," and "-"
# The following explains what happens if you provide a ","
# Set the Compose apps to none for devices, this will make the device run no apps:
fioctl config updates --apps ,

# The following explains what happens if you provide a "-"
# Set the Compose apps to preset-apps (all apps on most devices):
# The system will look in the following locations in order to get the complete config:
- /usr/lib/sota/conf.d/
- /var/sota/sota.toml
- /etc/sota/conf.d/
fioctl config updates --apps -

# Set the device tag to a preset-tag :
# The system will look in the following locations in order to get the complete config:
- /usr/lib/sota/conf.d/
- /var/sota/sota.toml
- /etc/sota/conf.d/
fioctl config updates --tag -`,
Copy link
Member

Choose a reason for hiding this comment

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

An indentation here seems rather weird.
Maybe, @kprosise can help with it?

At the least, these two sentences should be indented at the same level:

# The following explains what happens if you provide a ","
# The following explains what happens if you provide a "-"

The same applies to these three sentences (about indentation):

# Set the Compose apps to none for devices, this will make the device run no apps:
# Set the Compose apps to preset-apps (all apps on most devices):
# Set the device tag to a preset-tag :

Also, as sentences in these paragraphs are user-facing, they deserve a proper punctuation at their ends, like . or : (and proper spacing, like e.g. no space before a colon).

Copy link
Contributor

@kprosise kprosise left a comment

Choose a reason for hiding this comment

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

Provided suggestions based upon feedback from @vkhoroz.

Comment on lines +29 to +31
# There are two special characters: "," and "-"
# The following explains what happens if you provide a ","
# Set the Compose apps to none for devices, this will make the device run no apps:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# There are two special characters: "," and "-"
# The following explains what happens if you provide a ","
# Set the Compose apps to none for devices, this will make the device run no apps:
# There are two special characters: "," and "-".
# Providing a "," sets the Compose apps to "none" for devices.
# This will make the device run no apps:

Comment on lines +34 to +36
# The following explains what happens if you provide a "-"
# Set the Compose apps to preset-apps (all apps on most devices):
# The system will look in the following locations in order to get the complete config:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# The following explains what happens if you provide a "-"
# Set the Compose apps to preset-apps (all apps on most devices):
# The system will look in the following locations in order to get the complete config:
# Providing a "-" sets the Compose apps to "preset-apps" (all apps on most devices).
# The system looks in the following locations to get the complete config:

Comment on lines +42 to +43
# Set the device tag to a preset-tag :
# The system will look in the following locations in order to get the complete config:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Set the device tag to a preset-tag :
# The system will look in the following locations in order to get the complete config:
# Set the device tag to a "preset-tag",
# and the system will look in the following locations to get the complete config:

Comment on lines +30 to +32
# There are two special characters: "," and "-"
# The following explains what happens if you provide a ","
# Set the Compose apps to none, meaning it will run no apps:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# There are two special characters: "," and "-"
# The following explains what happens if you provide a ","
# Set the Compose apps to none, meaning it will run no apps:
# There are two special characters: "," and "-".
# Providing a "," sets the Compose apps to "none", meaning it will run no apps:

Comment on lines +35 to +37
# The following explains what happens if you provide a "-"
# Set the Compose apps to preset-apps (all apps on most devices):
# The system will look in the following locations in order to get the complete config:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# The following explains what happens if you provide a "-"
# Set the Compose apps to preset-apps (all apps on most devices):
# The system will look in the following locations in order to get the complete config:
# Providing a "-" sets the Compose apps to "preset-apps" (all apps on most devices).
# The system looks in the following locations to get the complete config:

Comment on lines +43 to +44
# Set the device tag to a preset-tag :
# The system will look in the following locations in order to get the complete config:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Set the device tag to a preset-tag :
# The system will look in the following locations in order to get the complete config:
# Set the device tag to a "preset-tag",
# and the system will look in the following locations to get the complete config:

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.

5 participants