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

Add refresh-bridges command #2573

Merged
merged 2 commits into from
Jan 10, 2024

Conversation

Bravo555
Copy link
Contributor

@Bravo555 Bravo555 commented Jan 9, 2024

Proposed changes

Add a tedge refresh-bridges command which regenerates and overwrites bridge configuration for all active bridges.

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

It was necessary to extract some stuff from connect module, so I did a small refactor in a first commit and the command implementation in the second. I expect there will be some blockers regarding the command itself (naming, and so on), so I could do the first commit as a separate PR.

Copy link

codecov bot commented Jan 9, 2024

Codecov Report

Attention: 184 lines in your changes are missing coverage. Please review.

Comparison is base (5615707) 75.9% compared to head (b127b2f) 75.8%.
Report is 23 commits behind head on main.

Additional details and impacted files
Files Coverage Δ
crates/core/tedge/src/bridge/aws.rs 98.1% <ø> (ø)
crates/core/tedge/src/bridge/azure.rs 98.1% <ø> (ø)
...ore/tedge/src/cli/connect/c8y_direct_connection.rs 0.0% <ø> (ø)
crates/core/tedge/src/cli/connect/cli.rs 0.0% <ø> (ø)
crates/core/tedge/src/cli/disconnect/cli.rs 39.1% <ø> (ø)
crates/core/tedge/src/cli/reconnect/cli.rs 0.0% <ø> (ø)
crates/core/tedge/src/cli/reconnect/command.rs 0.0% <ø> (ø)
crates/core/tedge/src/cli/mod.rs 72.7% <0.0%> (-1.0%) ⬇️
crates/core/tedge/src/bridge/c8y.rs 88.2% <97.4%> (ø)
crates/core/tedge/src/cli/common.rs 0.0% <0.0%> (ø)
... and 4 more

... and 14 files with indirect coverage changes

Copy link
Contributor

github-actions bot commented Jan 9, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
379 0 3 379 100 1h6m27.856s

Comment on lines +9 to 14
// XXX: having file name squished together with 20 fields which go into file content is a bit obscure
pub config_file: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

Fully agree. That said I would not spent too much time refactoring this bridge config. The long-term plan is to implement a direct connection from the mapper to the cloud - removing the need for an external bridge.

Comment on lines -8 to +10
const C8Y_CONFIG_FILENAME: &str = "c8y-bridge.conf";
const AZURE_CONFIG_FILENAME: &str = "az-bridge.conf";
const AWS_CONFIG_FILENAME: &str = "aws-bridge.conf";
use crate::bridge::AWS_CONFIG_FILENAME;
use crate::bridge::AZURE_CONFIG_FILENAME;
use crate::bridge::C8Y_CONFIG_FILENAME;
Copy link
Contributor

Choose a reason for hiding this comment

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

The fact that these constants where defined for connect, disconnect and reconnect is yet another sign that this module has rotten overtime.

Comment on lines +280 to 258
// FIXME: what does this magic number mean?
if token.contains("71") {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the C8Y smartrest code for a jwt token payload. See comment line 239.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, didn't notice the comment above, but still I'm inclined to leave that comment as a reminder that IMO we're using cumulocity smartrest codes magic numbers a bit too much, and having names for all of them defined in the c8y_api crate would be better.

crates/core/tedge/src/cli/refresh_bridges.rs Outdated Show resolved Hide resolved
configuration/package_scripts/tedge/postinst Show resolved Hide resolved
crates/core/tedge/src/cli/refresh_bridges.rs Show resolved Hide resolved
Copy link
Contributor

@albinsuresh albinsuresh left a comment

Choose a reason for hiding this comment

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

Apart from the major missing step: "mosquitto restart after the bridge configs are refreshed", the rest of the code looks fine.

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

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
@Bravo555 Bravo555 force-pushed the fix/2192/mosquitto-bridge-upgrade branch from 81bde11 to b127b2f Compare January 10, 2024 18:22
@Bravo555 Bravo555 added this pull request to the merge queue Jan 10, 2024
Merged via the queue into thin-edge:main with commit 190b6c9 Jan 10, 2024
19 checks passed
@Bravo555 Bravo555 deleted the fix/2192/mosquitto-bridge-upgrade branch January 11, 2024 12:13
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