-
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
Output tedge config control messages to stderr #1771
Conversation
Closes thin-edge#1721 Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
crates/core/tedge/tests/main.rs
Outdated
.stdout(predicate::str::contains("device.id")); | ||
.stderr(predicate::str::contains("device.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.
The change is correct ... but the test is misleading. My first understanding has been that the device.id would be printed on stderr.
.stdout(predicate::str::contains("device.id")); | |
.stderr(predicate::str::contains("device.id")); | |
.stderr(predicate::str::contains("'device.id' is not set")); |
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 like it's actually another error message:
failures:
---- tests::run_create_certificate stdout ----
thread 'tests::run_create_certificate' panicked at 'Unexpected stderr, failed var.contains('device.id' is not set)
├── var: The provided config key: 'device.id' is not configurable
└── var as str: The provided config key: 'device.id' is not configurable
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.
Oh true. This is read from the certificate. So let's check against is not configurable
.
3240d56
to
dde1d39
Compare
dde1d39
to
4f69471
Compare
Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
4f69471
to
514446c
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
@Bravo555 @didier-wenzek I have to updated some of the integration test bootstrap script as it is failing now due to this change (that is why the integration tests are failing). I'm working on the changes and reach out to @Bravo555 |
Robot Results
Passed Tests
|
* Output tedge config control messages to stderr Closes thin-edge#1721 Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com> * Print logs from tedge cli to stderr Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com> * Add tests Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com> --------- Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
Proposed changes
Move
tedge config
user directed messages to stderr so actual data output and user-directed messages can be more easily told apart and parsed as described in #1721.Types of changes
Paste Link to the issue
Checklist
cargo fmt
as mentioned in CODING_GUIDELINEScargo clippy
as mentioned in CODING_GUIDELINESFurther comments