tedge command for updating the service type #1798
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
|
@PradeepKiruvale Can you please add a robot test to validate this functionality? |
Robot Results
Passed Tests
|
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
|
@PradeepKiruvale I looked into the test failures and it seems that the setting that was added is mandatory (when it should be optional). It is important that if we add new configuration settings (sections or keys), it must be optional, as we don't want to break any existing configuration files out there (e.g. the classic upgrade scenario). I came to this conclusion based on the following log entries found in the last test run (pulled from the mapper logs) |
albinsuresh
reviewed
Mar 10, 2023
| @@ -469,3 +469,16 @@ impl ConfigSetting for FirmwareChildUpdateTimeoutSetting { | |||
|
|
|||
| type Value = Seconds; | |||
| } | |||
|
|
|||
| pub struct ServiceSetting; | |||
There was a problem hiding this comment.
Suggested change
| pub struct ServiceSetting; | |
| pub struct ServiceTypeSetting; |
| @@ -118,6 +121,7 @@ impl From<&TEdgeConfigLocation> for TEdgeConfigDefaults { | |||
| default_firmware_child_update_timeout: Seconds( | |||
| DEFAULT_FIRMWARE_CHILD_UPDATE_TIMEOUT_SEC, | |||
| ), | |||
| default_service_type: "service".into(), | |||
There was a problem hiding this comment.
You could just define this as a constant and reuse it here and in all tests.
| pub fn convert_health_status_message( | ||
| message: &Message, | ||
| device_name: String, | ||
| service_type: String, |
There was a problem hiding this comment.
Suggested change
| service_type: String, | |
| default_service_type: String, |
Just to be clearer as I was really struggling to follow the code below with the combination of service_type, health_status.service_type and s_type in the same context ;-)
albinsuresh
reviewed
Mar 10, 2023
| @@ -908,6 +908,7 @@ fn dummy_tedge_config_defaults() -> TEdgeConfigDefaults { | |||
| default_http_bind_address: IpAddress(IpAddr::V4(Ipv4Addr::LOCALHOST)), | |||
| default_c8y_smartrest_templates: TemplatesSet::default(), | |||
| default_firmware_child_update_timeout: Seconds(3600), | |||
| default_service_type: String::from("service"), | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Update some existing tests like test_parse_config_empty_file, test_parse_config_with_all_values and test_store_config_with_all_values to verify the effects of this new setting.
PradeepKiruvale
force-pushed
the
feature/service-type
branch
from
fc31044
to
96b58e3
Compare
Fixed by adding a default value. |
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
albinsuresh
reviewed
Mar 13, 2023
| @@ -573,7 +581,7 @@ fn test_parse_config_empty_file() -> Result<(), TEdgeConfigError> { | |||
| default_azure_root_cert_path: FilePath::from("/etc/ssl/certs"), | |||
| ..dummy_tedge_config_defaults() | |||
| }; | |||
|
|
|||
| dbg!(&config_defaults); | |||
There was a problem hiding this comment.
Suggested change
| dbg!(&config_defaults); |
| @@ -146,3 +149,9 @@ pub(crate) struct PathConfigDto { | |||
| pub(crate) struct FirmwareConfigDto { | |||
| pub(crate) child_update_timeout: Option<u64>, | |||
| } | |||
|
|
|||
| #[derive(Debug, Default, Deserialize, Serialize)] | |||
| pub(crate) struct ServiceConfigDto { | |||
There was a problem hiding this comment.
Suggested change
| pub(crate) struct ServiceConfigDto { | |
| pub(crate) struct ServiceTypeConfigDto { |
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
|
Yet another PR that convince me that something is terribly wrong with |
albinsuresh
reviewed
Mar 14, 2023
There was a problem hiding this comment.
The rust code looks good. But I'm not fully convinced with the test structure. Can @reubenmiller or @gligorisaev review the integration test and confirm if it's fine?
| @@ -31,6 +31,15 @@ Test if all c8y services are down | |||
| c8y-configuration-plugin | |||
| c8y-log-plugin | |||
|
|
|||
|
|
|||
| Test if all c8y services are using configured service type | |||
| [Template] Check if a service using configured service type | |||
There was a problem hiding this comment.
This test is probably okay but structure of this test looks problematic. This Check if a service using configured service type performs the tedge config set and tedge reconnect as part of this keyword and those commands will be called repeatedly for each service name argument that is passed to it(tedge-mapper-c8y, tedge-agent etc). That would unnecessarily call tedge reconnect for each service which is completely unnecessary, right? Why not do the tedge config set and tedge reconnect once as part of the test itself and then call the template that validates the service type for each input?
There was a problem hiding this comment.
The tedge reconnect is not needed here, just restarting the tedge-mapper-c8y is enough to pick up the service type config that is configured. I have updated the test.
Since the template pattern is used here, there is a restriction to call the custom test settings before starting these tests at once.
albinsuresh
reviewed
Mar 14, 2023
| tedge-mapper-c8y | ||
| tedge-agent | ||
| c8y-configuration-plugin | ||
| c8y-log-plugin |
There was a problem hiding this comment.
Suggested change
| c8y-log-plugin | |
| c8y-log-plugin | |
| c8y-firmware-plugin |
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
rina23q
reviewed
Mar 14, 2023
tests/RobotFramework/tests/cumulocity/service_monitoring/service_monitoring.robot
Outdated
Show resolved
Hide resolved
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
albinsuresh
approved these changes
Mar 16, 2023
| c8y-log-plugin | ||
| c8y-firmware-plugin | ||
|
|
||
| Test if all c8y services when service type configured as empty |
There was a problem hiding this comment.
Suggested change
| Test if all c8y services when service type configured as empty | |
| Test if all c8y services using default service type when service type configured as empty |
| fn default_status() -> String { | ||
| "unknown".to_string() | ||
| } | ||
|
|
||
| fn default_type() -> String { | ||
| "".to_string() |
There was a problem hiding this comment.
Suggested change
| "".to_string() | |
| DEFAULT_SERVICE_TYPE.to_string() |
This seems more appropriate even though I understand that the parsing logic below will convert this empty string to the default type anyway.
| Some(_) => {} | ||
| if health_status.service_type.is_empty() { | ||
| health_status.service_type = if default_service_type.is_empty() { | ||
| "service".to_owned() |
There was a problem hiding this comment.
Suggested change
| "service".to_owned() | |
| DEFAULT_SERVICE_TYPE.to_string() |
PradeepKiruvale
force-pushed
the
feature/service-type
branch
from
5dae171
to
3ad3e52
Compare
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
|
|
||
| The `default service type` can be configured using the `tedge` cli. | ||
|
|
||
| Below example shows how one can set the `service-type` to `systemd`. |
There was a problem hiding this comment.
Suggested change
| Below example shows how one can set the `service-type` to `systemd`. | |
| The example below shows how one can set the default service type to `systemd`. |
| > Note: When the `service type` was not sent with the `health status` message, then the configured default value will be used by | ||
| the mapper while translating the `health status` message to `service status` message. | ||
|
|
||
| To clear the configured default service type one can use the below command. |
There was a problem hiding this comment.
Suggested change
| To clear the configured default service type one can use the below command. | |
| To clear the configured default service type one can use the command below. |
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Signed-off-by: Pradeep Kumar K J <pradeepkumar.kj@softwareag.com>
PradeepKiruvale
force-pushed
the
feature/service-type
branch
from
63c8167
to
03011ec
Compare
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
didier-wenzek
pushed a commit
to didier-wenzek/thin-edge.io
that referenced
this pull request
Mar 21, 2023
Signed-off-by: Pradeep Kumar K J <pradeepkumar.kj@softwareag.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
This PR adds a feature to configure the
service_typethat is required for theservice monitoringfeature.Set service type :
The default value is
service.Types of changes
Paste Link to the issue
(#1769)
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments