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.

Sign up for GitHub

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

Jump to bottom

add http.client.host/port setting options #2411

Merged
merged 2 commits into from
Nov 8, 2023
Merged

add http.client.host/port setting options #2411

merged 2 commits into from
Nov 8, 2023

Conversation

Bravo555
Copy link
Contributor

@Bravo555 Bravo555 commented Nov 3, 2023 •
edited

Proposed changes

This commit introduces new http.client.host and http.client.port setting options. Similarly to the mqtt.client.* options, there is a significant difference between an address used to bind, e.g. "0.0.0.0" and an address used to connect.

http.client.host is now a string, which makes it possible to use DNS names where previously only IP address could be used.

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

#2391

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

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 3, 2023 •
edited

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
357 0 3 357 100 1h5m28.805s

@Bravo555 Bravo555 self-assigned this Nov 6, 2023
@Bravo555 Bravo555 force-pushed the fix/2390/decouple-mapper-c8y-from-agent branch from 149feb7 to 1d40ddb Compare November 6, 2023 15:22
@Bravo555 Bravo555 marked this pull request as ready for review November 6, 2023 15:22
@Bravo555 Bravo555 removed their assignment Nov 6, 2023
@codecov Codecov
Copy link

codecov bot commented Nov 6, 2023 •
edited

Codecov Report

Merging #2411 (ac3041c) into main (9b1ce24) will increase coverage by 0.0%.
Report is 6 commits behind head on main.
The diff coverage is 64.7%.

Additional details and impacted files
Files Coverage Δ
crates/extensions/c8y_config_manager/src/tests.rs 89.2% <100.0%> (ø)
...rates/extensions/c8y_firmware_manager/src/tests.rs 91.0% <100.0%> (ø)
crates/extensions/c8y_log_manager/src/actor.rs 75.5% <100.0%> (ø)
.../tedge_config/src/tedge_config_cli/tedge_config.rs 82.0% <87.5%> (-0.1%) ⬇️
...ates/extensions/c8y_firmware_manager/src/config.rs 57.5% <50.0%> (ø)
crates/extensions/c8y_log_manager/src/config.rs 0.0% <0.0%> (ø)
crates/extensions/c8y_config_manager/src/config.rs 60.6% <40.0%> (ø)
crates/extensions/c8y_mapper_ext/src/config.rs 46.2% <25.0%> (ø)

... and 7 files with indirect coverage changes

@Bravo555 Bravo555 self-assigned this Nov 7, 2023
@reubenmiller reubenmiller changed the title add http.client.address/port setting options add http.client.host/port setting options Nov 7, 2023
@Bravo555 Bravo555 force-pushed the fix/2390/decouple-mapper-c8y-from-agent branch from 1d40ddb to b9362b3 Compare November 7, 2023 14:08
@Bravo555 Bravo555 force-pushed the fix/2390/decouple-mapper-c8y-from-agent branch from b9362b3 to 8480ec8 Compare November 7, 2023 14:29
@Bravo555 Bravo555 force-pushed the fix/2390/decouple-mapper-c8y-from-agent branch from 8480ec8 to a852aff Compare November 7, 2023 14:41
@Bravo555 Bravo555 removed their assignment Nov 7, 2023
reubenmiller
reubenmiller requested changes Nov 7, 2023
View reviewed changes
Copy link
Contributor

@reubenmiller reubenmiller left a comment

Choose a reason for hiding this comment

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

Added some comments, and main point is to remove the skipping of the system test (we should be doing this in a different PR).

reubenmiller
reubenmiller approved these changes Nov 7, 2023
View reviewed changes
Copy link
Contributor

@reubenmiller reubenmiller left a comment

Choose a reason for hiding this comment

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

Approved though please wait for @jarhodes314 to review it again

@Bravo555
feat: add http.client.host/port setting options
1554ec1 
This commit introduces new `http.client.host` and `http.client.port`
setting options. Similarly to the `mqtt.client.*` options, there is a
significant difference between an address used to bind, e.g. "0.0.0.0"
and an address used to connect.

`http.client.host` is now a string, which makes it possible to use DNS
names where previously only IP address could be used.
@Bravo555
docs: update parts about the file transfer address
ac3041c
@Bravo555 Bravo555 force-pushed the fix/2390/decouple-mapper-c8y-from-agent branch from 4ef3acc to ac3041c Compare November 8, 2023 10:20
@Bravo555 Bravo555 merged commit 731b47e into thin-edge:main Nov 8, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reubenmiller reubenmiller reubenmiller approved these changes

@jarhodes314 jarhodes314 jarhodes314 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Bravo555 @reubenmiller @jarhodes314