-
Notifications
You must be signed in to change notification settings - Fork 16
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
base: main
Are you sure you want to change the base?
feat(config): make it so you can revert override of apps #440
Conversation
Tangential note: we don't need to support |
7d7142f
to
854ec25
Compare
854ec25
to
fa5568e
Compare
There was a problem hiding this 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
There was a problem hiding this 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.
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. |
fa5568e
to
5436ba9
Compare
Signed-off-by: Eric Bode <[email protected]>
5436ba9
to
c3d393d
Compare
There was a problem hiding this 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.
# 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 -`, |
There was a problem hiding this comment.
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).
There was a problem hiding this 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.
# 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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# 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: |
# 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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# 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: |
# Set the device tag to a preset-tag : | ||
# The system will look in the following locations in order to get the complete config: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# 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: |
# 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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# 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: |
# 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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# 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: |
# Set the device tag to a preset-tag : | ||
# The system will look in the following locations in order to get the complete config: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# 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: |
This is to add a way to disable the override, inherit from parent concerning apps running on devices.
Relevant fioconfig code