-
Notifications
You must be signed in to change notification settings - Fork 54
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
Filtering criteria in apt-sm-plugin output #1961
Conversation
c5ffa49
to
02b0d73
Compare
79f1422
to
450aae3
Compare
d4efaf0
to
ec66282
Compare
Robot Results
Passed Tests
|
eca9bdd
to
06979ce
Compare
0418613
to
5f88978
Compare
6d6d4a2
to
6eed099
Compare
|
||
const DESCRIPTION: &'static str = concat!( | ||
"The maximum number of software packages reported by each sm-plugin", | ||
"Note: If the value is set to 0, there is no limit to reported packages", |
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.
Using 0
to mean no-limit is usual but not really sound. Why not using an Option
? Or simpler, set a default of 1000
as suggested by #1926 ?
crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs
Outdated
Show resolved
Hide resolved
Filtering the output of software-package lists must be addressed at two levels.
This means for this PR:
@reubenmiller is this okay for you that the apt-plugin read its config from |
Yes I'm ok with this proposal for now. We might be a bit silent about the feature/docs until the support of the |
5f88978
to
a601661
Compare
a32a7df
to
2bdbf0f
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.
Nice, things are going in the expected direction.
070089b
to
50b2111
Compare
573fd17
to
2c63d4d
Compare
5bb0544
to
3646424
Compare
Config settings have now to be added to I acknowledge that the current status is confusing as there are still some pending cleaning tasks to finalize #1936. _=> I will take the time tomorrow to help you and see what needs to be cleaned.
|
I had fixed that issue just before you posted the comment. If we are after the tedge config refactor, should we now add one for name and maintainer filters? |
7943758
to
b617f12
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.
[EDITED] as I changed my mind after a small experiment: it's super easy to use the new tedge config right now.
The main feature is working fine, but some improvements are required before merging.
- The source of truth is now
tedge_config/../new.rs
and the old settings must no more be used. - The Quantity type can simply be removed as not really useful.
max_packages
doc and default value have to be fixed in the newtedge_config
.- error handling for ill-formed
name
ormaintainer
settings can be improved.
crates/common/tedge_config/src/tedge_config_cli/tedge_config_defaults.rs
Outdated
Show resolved
Hide resolved
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.
Suddenly dropping in and left some comments like ninja 🥷
crates/core/plugin_sm/src/plugin.rs
Outdated
self.name.clone(), | ||
output.stdout.as_slice(), | ||
)?) | ||
let config = get_tedge_config().unwrap(); |
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.
I don't think this unwrap()
is safe because if /etc/tedge/tedge.toml
doesn't exist or is malformed, it should panic.
crates/core/plugin_sm/src/plugin.rs
Outdated
output.stdout.as_slice(), | ||
)?) | ||
let config = get_tedge_config().unwrap(); | ||
let limit = config.query(SoftwarePluginMaxPackagesSetting).unwrap().0; |
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.
This unwrap()
is safe because config.query(SoftwarePluginMaxPackagesSetting)
returns always Ok
. However, it is always recommended to add a comment why it's safe when you use unwrap()
.
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.
I agree with @rina23q comment: using unwrap()
here is unsafe and also buggy as the config might be stored in a non-default location. The correct way to implement that would be to do the same as for the default plugin type: to let the agent read the config and provide the TEdgeConfig
value to the plugin. But as this PR is open since quite a long time, I'm okay to have this done in a following PR.
|
||
Apply both filters | ||
${packages_list}= Execute Command | ||
... sudo /etc/tedge/sm-plugins/apt list --name sudo --maintainer thin-edge.* |
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.
Why the name is sudo
? Using it as an example package name which doesn't exist?
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.
Actually, quite the opposite. Filters work here like union so I wanted to show that when we use it with the maintainer filter set to thin-edge, it will print not only all the tedge packages but also sudo package. And I think we can safely assume that the sudo package will be available (In fact, it is even used when executing the test command).
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.
Here the list of changes to make this PR using the new tedge config.
crates/common/tedge_config/src/tedge_config_cli/models/quantity.rs
Outdated
Show resolved
Hide resolved
crates/common/tedge_config/src/tedge_config_cli/tedge_config_defaults.rs
Outdated
Show resolved
Hide resolved
a3da089
to
e45708e
Compare
Signed-off-by: Krzysztof Piotrowski <Krzysztof.Piotrowski@inetum.com>
Signed-off-by: Krzysztof Piotrowski <Krzysztof.Piotrowski@inetum.com>
Signed-off-by: Krzysztof Piotrowski <Krzysztof.Piotrowski@inetum.com>
Signed-off-by: Krzysztof Piotrowski <Krzysztof.Piotrowski@inetum.com>
Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>
b61d54d
to
7f5cf86
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.
Approved.
Follow-up tasks:
- The
ExternalPluginCommand::list()
method must not assume the config is at the default location. - Consider to manage the
name
andmaintainer
filters withtedge config
.
Proposed changes
This PR adds two features:
tedge.toml
.tedge config
. Default value is set to 1000 and 0 means no limit to the output (STC).Types of changes
Paste Link to the issue
#1926
Checklist
cargo fmt
as mentioned in CODING_GUIDELINEScargo clippy
as mentioned in CODING_GUIDELINESFurther comments