-
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
Issue#1804 tedge config setting for data path #1807
Issue#1804 tedge config setting for data path #1807
Conversation
Robot Results
Passed Tests
|
b3f8ec3
to
c16e852
Compare
c16e852
to
5ecea4a
Compare
@@ -90,6 +94,7 @@ impl From<&TEdgeConfigLocation> for TEdgeConfigDefaults { | |||
let tmp_path = Path::new(DEFAULT_TMP_PATH); | |||
let logs_path = Path::new(DEFAULT_LOG_PATH); | |||
let run_path = Path::new(DEFAULT_RUN_PATH); | |||
let data_path = Path::new(DEFAULT_DATA_PATH); |
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 is not introduced by this PR, but this pattern is really heavy: a set of const
used to initialize local values and to finally build a TEdgeConfigDefault
struct which all fields are prefixed by a default
prefix.
Why not simply Impl Default for TEdgeConfig
?
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.
Makes sense. I'll give that a try.
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.
Reviewed. Happy to see that we get rid of many #[cfg(not(test))]
:)
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 code already looks good in my point of view.
870d841
to
102aa61
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.
This file named 018_change_temp_path.md
only had the documentation on updating tmp.path
. I replaced it with a generic one named 018_update_config_paths.md
that documents all paths that can be updated.
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.
Comments on doc.
66afe23
to
107d396
Compare
107d396
to
19c7ac1
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
#[cfg(test)] | ||
pub const FILE_TRANSFER_ROOT_PATH: &str = "/tmp"; | ||
#[cfg(not(test))] | ||
pub const FILE_TRANSFER_ROOT_PATH: &str = "/var/tedge/file-transfer"; |
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.
Lesson to be learned with this issue & PR. Having different values for a const
, while under test or not, is a sign that something is definitely 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.
Looks all good now :)
19c7ac1
to
a497e8a
Compare
393a5ea
to
622cd20
Compare
622cd20
to
9167d25
Compare
Sleep 3s | ||
Execute Command touch /etc/tedge/operations/c8y/${CHILD_SN}/c8y_Firmware | ||
Sleep 3s | ||
Cumulocity.Device Should Exist ${CHILD_SN} |
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.
We can make the test deterministic by removing the above sleep
statements. Isn't it Cumulocity. Device Should Exist
is having retries to check whether the device is already created. If not may be this needs support from the library.
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.
Unfortunately the Device Should Exist
does not retry that check. The purpose of those sleeps are really not only to make sure that the device gets created, but to make sure that the device gets created with the right name. Since we're relying on inotify mechanism to get the child device created, as well as to update its supported operations, the order in which these steps are executed is critical to make sure that the device gets created with the right name (without the Mqtt device prefix) and metadata.
This definitely highlights the need for more such deterministic APIs in the Cumulocity
library so that we can conditionally wait on some inventory state changes. But, it's slightly out of the scope of this ticket. So, it'd be better to address that specifically in a dedicated "test framework enhancement PR".
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.
Thanks for the clarification. I am ok to implement this as part of another PR.
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 Device Should Exist
will work just fine there, as the external id is fine, it is just the name of the device which is not ok sometimes....But in your instance you don't really care about the name, just the external id.
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.
Also there are other checks which allow waiting for inventory fragment changes...Or are you referring to wait for a child device (which is technically a linked inventory and not a custom fragment)?
Cumulocity.Device Should Have Fragments
Cumulocity.Device Should Have Fragment Values
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.
Glad to know that APIs like Device Should Have Fragments
also exist. But from what I understood, these APIs check for the desired state only once. What we'd need on top of these existing APIs, to avoid these sleeps, is a "timed waiting" semantic, (similar to the functionality offered by this waiting package), with which we can conditionally wait for an inventory change(device created, desired fragment added etc) with a max timeout value.
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.
As a general rule of thumb, all assertions are dynamic in nature, and rerun an assertion until it is true, or if the timeout interval has been reached.
9167d25
to
f5d22b5
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.
I just had a look at the integration
tests, LGTM.
Proposed changes
Types of changes
Paste Link to the issue
#1804 #1732
Checklist
cargo fmt
as mentioned in CODING_GUIDELINEScargo clippy
as mentioned in CODING_GUIDELINESFurther comments