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

fix(#2445): add c8y proxy config settings #2446

Merged

Conversation

Bravo555
Copy link
Contributor

@Bravo555 Bravo555 commented Nov 13, 2023

TODO

  • documentation

Proposed changes

This PR adds c8y.proxy.client.host and c8y.proxy.client.port used to connect to running C8y HTTP proxy.

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

@Bravo555 Bravo555 requested review from reubenmiller and jarhodes314 and removed request for reubenmiller and jarhodes314 November 13, 2023 12:42
@Bravo555 Bravo555 force-pushed the fix/2445/independent-c8y-proxy-settings branch from da17827 to d0ffd90 Compare November 13, 2023 12:59
Copy link

codecov bot commented Nov 13, 2023

Codecov Report

Merging #2446 (eafe1b3) into main (02dc80d) will decrease coverage by 0.1%.
Report is 6 commits behind head on main.
The diff coverage is 73.0%.

Additional details and impacted files
Files Coverage Δ
crates/extensions/c8y_mapper_ext/src/actor.rs 77.6% <ø> (+0.8%) ⬆️
crates/extensions/c8y_mapper_ext/src/tests.rs 91.6% <100.0%> (+<0.1%) ⬆️
crates/extensions/c8y_mapper_ext/src/converter.rs 81.1% <50.0%> (+<0.1%) ⬆️
.../tedge_config/src/tedge_config_cli/tedge_config.rs 82.0% <85.7%> (+<0.1%) ⬆️
crates/extensions/c8y_auth_proxy/src/url.rs 81.8% <66.6%> (ø)
crates/extensions/c8y_mapper_ext/src/config.rs 46.3% <33.3%> (+0.1%) ⬆️

... and 4 files with indirect coverage changes

@Bravo555 Bravo555 temporarily deployed to Test Pull Request November 13, 2023 13:08 — with GitHub Actions Inactive
Copy link
Contributor

github-actions bot commented Nov 13, 2023

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
359 0 3 359 100 55m17.061s

Comment on lines +14 to +16
The Cumulocity HTTP API can be accessed at `http://{host}:{port}/c8y/{c8y-endpoint}`. Configuration settings
`c8y.proxy.client.host` and `c8y.proxy.client.port` are used to configure `{host}` and `{port}` parts of the base URL
which will be used by thin-edge components to make requests to the C8y Proxy. `c8y.proxy.bind.address` and
Copy link
Contributor

Choose a reason for hiding this comment

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

If my understanding is correct, this should stress that the client settings configure the mapper and not the agent (which is what I, as a user, would assume the client setting changes as it is how the equivalent MQTT settings work). I guess the main clue as to this being the case without knowing the implementation details is that the c8y settings only affect the mapper as that's the only component that cares about Cumulocity.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is defining how clients can reach the endpoint.

The mapper is the only component that uses the setting directly, however the value of the setting will be used within command payloads which are consumed by other clients (e.g. custom device connectors)

Copy link
Contributor

Choose a reason for hiding this comment

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

I get that, my point was "there isn't any point setting this on a child device as the value won't be used", which is unlike what happens for the mqtt client settings. It probably doesn't have much of an impact, but the logic behind why it's like that doesn't seem like a thing that's necessarily obvious to a user so might be worth drawing attention to.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @jarhodes314. We must be explicit telling that both c8y.proxy.bind.address and c8y.proxy.client.host have to be set on the box running the c8y mapper. The latter will be forwarded by the c8y mapper to its clients. c8y.proxy.client.host can also be directly set on the clients which aim to use the c8y REST API.

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, forgot about this ticket. Description tweaked in 0159f91, is it OK now?

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

This commit adds `c8y.proxy.client.host` and `c8y.proxy.client.port`
used to connect to running C8y HTTP proxy.
@Bravo555 Bravo555 force-pushed the fix/2445/independent-c8y-proxy-settings branch from 0159f91 to eafe1b3 Compare November 14, 2023 18:48
@Bravo555 Bravo555 temporarily deployed to Test Pull Request November 14, 2023 19:03 — with GitHub Actions Inactive
@Bravo555 Bravo555 merged commit 35add19 into thin-edge:main Nov 14, 2023
@Bravo555 Bravo555 deleted the fix/2445/independent-c8y-proxy-settings branch November 14, 2023 22: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.

4 participants