-
Notifications
You must be signed in to change notification settings - Fork 70
feat: support disabling cloud handling of c8y_Restart operation #3940
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: support disabling cloud handling of c8y_Restart operation #3940
Conversation
| /// Enable device restart feature | ||
| #[tedge_config(example = "true", default(value = true))] | ||
| device_restart: bool, |
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'm not sure whether we should use device_restart or just restart. I opted for a more explicit approach given that some time in the future we'll probably have a "service_restart" command.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
Robot Results
|
d69a5a2 to
9894711
Compare
| Execute Command tedge config set c8y.enable.device_restart false | ||
| Execute Command rm -f /etc/tedge/operations/c8y/c8y_Restart |
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.
Does a user need to do both these actions to disable the operation, or is deleting the file just done so we can verify it doesn't reappear after restarting the mapper?
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.
Yes I think the file still needs to be deleted if the tedge-mapper-c8y service has already been started (with the setting on)...though I didn't verify this, so maybe I'm wrong.
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 just confirmed that once the c8y_Restart file is created, then the user has to manually remove it as disabling the feature does not affect the supported operations list that is sent to Cumulocity. However the mapper won't react to the operation if it is sent to the cloud.
We could delete the operation if present on startup though. Previously we avoided this as we thought that people might want to customize the operation handler (e.g. create the file themselves), however I don't think anyone is doing this now that we can customize the tedge-agent workflows instead.
The code shows that there is only an "add_operation" function, and nothing to remove an already existing one.
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. For the sake of completeness, shouldn't we do the same for software update operation as well?
rina23q
left a comment
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. I just found a trivial thing that can be better.
Signed-off-by: reubenmiller <[email protected]>
53259f7 to
9b8eaac
Compare
Proposed changes
Support new tedge configuration (
c8y.enable.device_restart) to disable the handling of the thec8y_Restartoperation from Cumulocity.Other Cumulocity cloud operations already support this mechanism, so the PR just aligns the c8y_Restart with the other cloud operation handlers.
Types of changes
Paste Link to the issue
#3939
Checklist
just prepare-devonce)just formatas mentioned in CODING_GUIDELINESjust checkas mentioned in CODING_GUIDELINESFurther comments