Skip to content
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

Gracefully handle unknown config #1632

Merged

Conversation

cmosd
Copy link
Contributor

@cmosd cmosd commented Dec 2, 2022

Proposed changes

This PR:

  • adds a new crate inside an existing crate: "tedge-derive" behind a feature flag
  • uses this crate to catch unknown serde values, and stores them under a new field named other.
  • adds a new declarative macro to generate warnings when any toml fields have unknown values.
  • turns on logging for the tedge binary

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s)
  • I ran cargo fmt as mentioned in CODING_GUIDELINES
  • I used cargo clippy as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@cmosd
Copy link
Contributor Author

cmosd commented Dec 2, 2022

An example of the warning messages:
image

These messages are triggered any time the toml is read. Meaning it is not just triggered by the tedge binary but also by all the other binaries reading the "tedge.toml" file.

@cmosd cmosd self-assigned this Dec 2, 2022
@cmosd cmosd marked this pull request as ready for review December 2, 2022 16:33
@cmosd cmosd force-pushed the improvement/1623/gracefully-handle-unknown-config branch from d273571 to 0c65888 Compare December 2, 2022 16:36
@cmosd cmosd force-pushed the improvement/1623/gracefully-handle-unknown-config branch from 0c65888 to 0adc184 Compare December 2, 2022 16:37
cmosd pushed a commit to cmosd/thin-edge.io that referenced this pull request Dec 2, 2022
Signed-off-by: alexandru solomes <solo@softwareag.com>
@cmosd cmosd force-pushed the improvement/1623/gracefully-handle-unknown-config branch from f54d302 to e28b718 Compare December 2, 2022 19:51
cmosd pushed a commit to cmosd/thin-edge.io that referenced this pull request Dec 2, 2022
Signed-off-by: alexandru solomes <solo@softwareag.com>
@cmosd cmosd force-pushed the improvement/1623/gracefully-handle-unknown-config branch from e28b718 to 6766056 Compare December 2, 2022 19:52
cmosd pushed a commit to cmosd/thin-edge.io that referenced this pull request Dec 3, 2022
Signed-off-by: alexandru solomes <solo@softwareag.com>
@cmosd cmosd force-pushed the improvement/1623/gracefully-handle-unknown-config branch from 7667160 to a8f2af7 Compare December 3, 2022 15:54
cmosd pushed a commit to cmosd/thin-edge.io that referenced this pull request Dec 5, 2022
Signed-off-by: alexandru solomes <solo@softwareag.com>
@cmosd cmosd force-pushed the improvement/1623/gracefully-handle-unknown-config branch from a8f2af7 to 89c07e5 Compare December 5, 2022 14:48
@@ -19,6 +19,7 @@ const BROKER_GROUP: &str = "mosquitto";

fn main() -> anyhow::Result<()> {
let opt = cli::Opt::parse();
tedge_utils::logging::initialise_tracing_subscriber(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is not it should be like below, this also needs to pass --debug flag when one uses the tedge cli.

Suggested change
tedge_utils::logging::initialise_tracing_subscriber(false);
tedge_utils::logging::initialise_tracing_subscriber(opt.debug);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be updated when merged with #1624.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this, we need to add --debug option for tedge command. This can be a separate PR.

Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

alexandru solomes added 4 commits December 7, 2022 09:58
New macro crate that introduces the `serde_other` proc macro attribute
for capturing unknown fields when deserialising to a struct.

Signed-off-by: alexandru solomes <solo@softwareag.com>
Signed-off-by: alexandru solomes <solo@softwareag.com>
Added logging to `tedge` and warning is displayed when they toml field
or keys are not known.

Signed-off-by: alexandru solomes <solo@softwareag.com>
Signed-off-by: alexandru solomes <solo@softwareag.com>
@cmosd cmosd force-pushed the improvement/1623/gracefully-handle-unknown-config branch from 89c07e5 to ab7d89b Compare December 7, 2022 09:59
@cmosd cmosd merged commit b7f8109 into thin-edge:main Dec 7, 2022
@cmosd cmosd deleted the improvement/1623/gracefully-handle-unknown-config branch December 7, 2022 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants