-
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
tedge_mapper az/collectd services start running when the thin-edge device restarts #1683
tedge_mapper az/collectd services start running when the thin-edge device restarts #1683
Conversation
ffbd3a6
to
04a9aea
Compare
|
||
[Service] | ||
User=tedge | ||
ExecStartPre=/bin/bash -c '/usr/bin/systemctl is-active --quiet collectd.service' |
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.
It is better to use the Requires
field to list the hard dependency to the collectd.service
instead of using the ExecStartPre
.
Checkout the systemd docs for more info
crates/core/tedge_mapper/Cargo.toml
Outdated
start = false | ||
enable = false |
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.
Don't we want to enable the starting of the service by default now? As the other services should be prevented from starting due to the ConditionPathExists
which will block it from starting if the related mapper files don'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.
Yup, Removed these.
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.
Few minor changes required
Signed-off-by: Pradeep Kumar K J <pradeepkumar.kj@softwareag.com>
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.
Looks good, commit messages should be cleaned up before merging though.
Also, better to test the new artifacts before merging especially to check if it doesn't break self-update of tedge_mapper. @gligorisaev has the test case for that already.
15fedf4
to
2e73de6
Compare
Just updating the expected test conditions: The systemctl service status For example:
|
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. I verified the test case with @gligorisaev and we have the 👍
Proposed changes
Disable the
tedge-mapper az/c8y/collectd
on installationAlso, start the mappers only if bridge config file exists
Types of changes
Paste Link to the issue
#1631
Checklist
cargo fmt
as mentioned in CODING_GUIDELINEScargo clippy
as mentioned in CODING_GUIDELINESFurther comments